[edk2-devel] [RFC] Introduce new status code for ManageabilityPkg and Redfish*Pkg

[Nickle Wang via groups.io]

Hi edk2 community,

I like to introduce EFI_COMPUTING_UNIT_MANAGEABILITY status code to 
PiStatusCode.h. EFI_COMPUTING_UNIT_MANAGEABILITY will be used in edk2 
RedfishPkg and edk2-redfish-client RedfishClientPkg to report Redfish operation 
errors. It will also be used to report errors in edk2-platforms 
ManageabilityPkg.

EFI_COMPUTING_UNIT_MANAGEABILITY is created as one of the subclasses in 
computing unit class.

#define EFI_COMPUTING_UNIT_CACHE   (EFI_COMPUTING_UNIT | 0x0004)
#define EFI_COMPUTING_UNIT_MEMORY  (EFI_COMPUTING_UNIT | 0x0005)
#define EFI_COMPUTING_UNIT_CHIPSET (EFI_COMPUTING_UNIT | 0x0006)
+ #define EFI_COMPUTING_UNIT_MANAGEABILITY   (EFI_COMPUTING_UNIT | 
0x0007)

Below operation values are defined to report failure in manageability related 
operations. I only provide the definitions for Redfish functions but the 
failure case like in MCTP, IPMI and KCS can be created in the future.

+///
+/// Computing Unit Manageability Subclass Error Code definitions.
+/// The detail information is reported by REPORT_STATUS_CODE_WITH_EXTENDED_DATA
+//  with ASCII string in EFI_STATUS_CODE_STRING_DATA.
+///@{
+#define EFI_MANAGEABILITY_EC_REDFISH_COMMUNICATION_ERROR
(EFI_SUBCLASS_SPECIFIC | 0x)
+#define EFI_MANAGEABILITY_EC_REDFISH_HOST_INTERFACE_ERROR   
(EFI_SUBCLASS_SPECIFIC | 0x0001)
+#define EFI_MANAGEABILITY_EC_REDFISH_BOOTSTRAP_CREDENTIAL_ERROR 
(EFI_SUBCLASS_SPECIFIC | 0x0002)

* EFI_MANAGEABILITY_EC_REDFISH_COMMUNICATION_ERROR will be used to report 
communication failure between host and Redfish service providers (or BMC).
* EFI_MANAGEABILITY_EC_REDFISH_HOST_INTERFACE_ERROR is reported when host 
system can not create Redfish host interface due to some errors.
* EFI_MANAGEABILITY_EC_REDFISH_BOOTSTRAP_CREDENTIAL_ERROR is reported when host 
system cannot get bootstrap credentials by following the Host Interface 
standard.

Detail reason will be provided in ASCII string by calling 
REPORT_STATUS_CODE_WITH_EXTENDED_DATA().

The pull request is here for reference: 
https://github.com/nicklela/edk2/pull/4/files  Any feedback is welcome.

Thanks,
Nickle




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105525): https://edk2.groups.io/g/devel/message/105525
Mute This Topic: https://groups.io/mt/99257555/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] UnitTestFrameworkPkg: Add UnitTestPeiServicesTablePointerLib

[Zhiguang Liu]

This library supports a PeiServicesTablePointerLib implementation
that allows code dependent upon PeiServicesTable to operate in an
isolated execution environment such as within the context of a
host-based unit test framework.

The unit test should initialize the PeiServicesTable database with
any required elements (e.g. PPIs, Hob etc.) prior to the services
being invoked by code under test.

It is strongly recommended to clean any global databases by using
EFI_PEI_SERVICES.ResetSystem2 after every unit test so the tests
execute in a predictable manner from a clean state.

Cc: Michael D Kinney 
Cc: Michael Kubacki 
Cc: Sean Brogan 
Signed-off-by: Zhiguang Liu 
---
 .../UnitTestPeiServicesTablePointerLib.c  | 215 ++
 .../UnitTestPeiServicesTablePointerLib.h  | 658 ++
 .../UnitTestPeiServicesTablePointerLib.inf|  38 +
 .../UnitTestPeiServicesTablePointerLib.uni|  12 +
 .../UnitTestPeiServicesTablePointerLibHob.c   | 162 +
 .../UnitTestPeiServicesTablePointerLibMisc.c  | 430 
 .../UnitTestPeiServicesTablePointerLibPpi.c   | 485 +
 UnitTestFrameworkPkg/UnitTestFrameworkPkg.dsc |   1 +
 .../UnitTestFrameworkPkgHost.dsc.inc  |   1 +
 9 files changed, 2002 insertions(+)
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.c
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.h
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.inf
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.uni
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLibHob.c
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLibMisc.c
 create mode 100644 
UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLibPpi.c

diff --git 
a/UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.c
 
b/UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.c
new file mode 100644
index 00..62ba0048dd
--- /dev/null
+++ 
b/UnitTestFrameworkPkg/Library/UnitTestPeiServicesTablePointerLib/UnitTestPeiServicesTablePointerLib.c
@@ -0,0 +1,215 @@
+/** @file
+  This library supports a PEI Service table Pointer library implementation that
+  allows code dependent upon PEI Service to operate in an isolated execution 
environment
+  such as within the context of a host-based unit test framework.
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "UnitTestPeiServicesTablePointerLib.h"
+
+#define MAX_PPI_COUNT  100
+#define MAX_HOB_SIZE   SIZE_32MB
+
+///
+/// Pei service instance
+///
+EFI_PEI_SERVICES  mPeiServices = {
+  {
+PEI_SERVICES_SIGNATURE,
+PEI_SERVICES_REVISION,
+sizeof (EFI_PEI_SERVICES),
+0,
+0
+  },
+  UnitTestInstallPpi,   // InstallPpi
+  UnitTestReInstallPpi, // ReInstallPpi
+  UnitTestLocatePpi,// LocatePpi
+  UnitTestNotifyPpi,// NotifyPpi
+
+  UnitTestGetBootMode,  // GetBootMode
+  UnitTestSetBootMode,  // SetBootMode
+
+  UnitTestGetHobList, // GetHobList
+  UnitTestCreateHob,  // CreateHob
+
+  UnitTestFfsFindNextVolume,  // FfsFindNextVolume
+  UnitTestFfsFindNextFile,// FfsFindNextFile
+  UnitTestFfsFindSectionData, // FfsFindSectionData
+
+  UnitTestInstallPeiMemory, // InstallPeiMemory
+  UnitTestAllocatePages,// AllocatePages
+  UnitTestAllocatePool, // AllocatePool
+  (EFI_PEI_COPY_MEM)CopyMem,
+  (EFI_PEI_SET_MEM)SetMem,
+
+  UnitTestReportStatusCode, // ReportStatusCode
+  UnitTestResetSystem,  // ResetSystem
+
+  NULL,  // CpuIo
+  NULL,  // PciCfg
+
+  UnitTestFfsFindFileByName,   // FfsFindFileByName
+  UnitTestFfsGetFileInfo,  // FfsGetFileInfo
+  UnitTestFfsGetVolumeInfo,// FfsGetVolumeInfo
+  UnitTestRegisterForShadow,   // RegisterForShadow
+  UnitTestFfsFindSectionData3, // FfsFindSectionData3
+  UnitTestFfsGetFileInfo2, // FfsGetFileInfo2
+  UnitTestResetSystem2,// ResetSystem2
+  UnitTestFreePages,   // FreePages
+};
+
+EFI_PEI_SERVICES   *mPeiServicesPointer = 
+BOOLEANPSInitDone   = FALSE;
+PEI_CORE_INSTANCE  mPrivateData;
+VOID   *mHobBuffer;
+VOID   *mPpiBuffer;
+VOID   *mPpiNotifyBuffer;
+
+/**
+  Allocate Buffer Or Clear Buffer For Global Data.
+**/
+VOID
+AllocateBufferOrClearBufferForGlobalData (
+  VOID
+  )
+{
+  ZeroMem (, sizeof (mPrivateData));
+  if (!PSInitDone) {
+mHobBuffer = AllocatePages (EFI_SIZE_TO_PAGES (MAX_HOB_SIZE));
+ASSERT (mHobBuffer != NULL);
+
+mPpiBuffer = AllocateZeroPool (sizeof 

Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Get processor extended information for SmmCpuServiceProtocol

[Ni, Ray]

Reviewed-by: Ray Ni 

> -Original Message-
> From: Wu, Jiaxin 
> Sent: Thursday, June 1, 2023 9:31 AM
> To: Zhang, Hongbin1 ; devel@edk2.groups.io
> Cc: Dong, Eric ; Ni, Ray ; Kumar,
> Rahul R ; Gerd Hoffmann ;
> Zeng, Star 
> Subject: RE: [PATCH v1] UefiCpuPkg: Get processor extended information for
> SmmCpuServiceProtocol
> 
> Reviewed-by: Jiaxin Wu 
> 
> > -Original Message-
> > From: Zhang, Hongbin1 
> > Sent: Monday, May 29, 2023 2:40 PM
> > To: devel@edk2.groups.io
> > Cc: Zhang, Hongbin1 ; Dong, Eric
> > ; Ni, Ray ; Kumar, Rahul R
> > ; Gerd Hoffmann ; Zeng,
> > Star ; Wu, Jiaxin 
> > Subject: [PATCH v1] UefiCpuPkg: Get processor extended information for
> > SmmCpuServiceProtocol
> >
> > Some features like RAS need to use processor extended information
> > under smm, So add code to support it
> >
> > Signed-off-by: Hongbin1 Zhang 
> > 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 (#105523): https://edk2.groups.io/g/devel/message/105523
Mute This Topic: https://groups.io/mt/99209786/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] UefiCpuPkg: Get processor extended information for SmmCpuServiceProtocol

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu 

> -Original Message-
> From: Zhang, Hongbin1 
> Sent: Monday, May 29, 2023 2:40 PM
> To: devel@edk2.groups.io
> Cc: Zhang, Hongbin1 ; Dong, Eric
> ; Ni, Ray ; Kumar, Rahul R
> ; Gerd Hoffmann ; Zeng,
> Star ; Wu, Jiaxin 
> Subject: [PATCH v1] UefiCpuPkg: Get processor extended information for
> SmmCpuServiceProtocol
> 
> Some features like RAS need to use processor extended information
> under smm, So add code to support it
> 
> Signed-off-by: Hongbin1 Zhang 
> 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 (#105522): https://edk2.groups.io/g/devel/message/105522
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] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.

[Wu, Jiaxin]

Thanks Jiewen & Ray.

Zhihao, could you try the suggestion from Jiewen? You can sync with me for 
detail.

Thanks,
Jiaxin 

> -Original Message-
> From: Ni, Ray 
> Sent: Thursday, June 1, 2023 9:07 AM
> To: Yao, Jiewen ; devel@edk2.groups.io; Wu, Jiaxin
> ; Li, Zhihao ; Gao, Liming
> ; kra...@redhat.com
> Cc: Wang, Jian J 
> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> add Ap rendezvous check before SmmSetVariable.
> 
> Jiewen,
> Good suggestion.
> This also resolve's Jiaxin's comments that check if the variable is 
> non-volatile.
> 
> Thanks,
> Ray
> 
> > -Original Message-
> > From: Yao, Jiewen 
> > Sent: Thursday, June 1, 2023 9:06 AM
> > To: devel@edk2.groups.io; Wu, Jiaxin ; Li, Zhihao
> > ; Gao, Liming ; Ni, Ray
> > ; kra...@redhat.com
> > Cc: Wang, Jian J 
> > Subject: RE: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> > add Ap rendezvous check before SmmSetVariable.
> >
> > Disabling flash is a silicon behavior.
> >
> > Can we do SmmWaitForAllProcessor() in FVB driver, or SPI driver ?
> >
> > Thank you
> > Yao, Jiewen
> >
> >
> >
> > > -Original Message-
> > > From: devel@edk2.groups.io  On Behalf Of Wu,
> > Jiaxin
> > > Sent: Thursday, June 1, 2023 9:03 AM
> > > To: devel@edk2.groups.io; Li, Zhihao ; Gao, Liming
> > > ; Ni, Ray ;
> > kra...@redhat.com
> > > Cc: Wang, Jian J 
> > > Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> > add
> > > Ap rendezvous check before SmmSetVariable.
> > >
> > > Hi All,
> > >
> > > I think we need this patch:
> > >
> > > There is a requirement that all CPU threads must in SMM for Non-Volatile
> > > variable. Because the SMM will disables the flash protection. Before that,
> we
> > > must guarantee all CPU threads are in SMM to avoid the non-smm mode
> > cpus
> > > modify the flash.
> > >
> > >
> > > Zhihao,
> > >
> > > I think this is only needed for the Non-Volatile, I suggest as below 
> > > check:
> > >
> > >   if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> > > if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
> > >   DEBUG ((DEBUG_ERROR, " SmmVariableSetVariable: Fail to wait for
> all
> > AP
> > > check in SMM!\n"));
> > >   Status = EFI_ABORTED;
> > >   goto EXIT;
> > > }
> > >   }
> > >
> > > Thanks,
> > > Jiaxin
> > >
> > > > -Original Message-
> > > > From: devel@edk2.groups.io  On Behalf Of Li,
> > > > Zhihao
> > > > Sent: Friday, May 19, 2023 4:11 PM
> > > > To: Gao, Liming ; devel@edk2.groups.io;
> Ni,
> > > > Ray ; kra...@redhat.com
> > > > Cc: Wang, Jian J 
> > > > Subject: 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 ++-
> > > > >
> > > >
> >
> 

Re: [edk2-devel] [Patch V4 05/15] UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.

[Ni, Ray]

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 3deb1ffd67..a25a96f68c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Page Fault (#PF) handler for X64 processors
> 
> -Copyright (c) 2009 - 2022, Intel Corporation. All rights reserved.
> +Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -353,7 +353,12 @@ SmmInitPageTable (
>m1GPageTableSupport   = Is1GPageSupport ();
>m5LevelPagingNeeded   = Is5LevelPagingNeeded ();
>mPhysicalAddressBits  = CalculateMaximumSupportAddress ();
> -  PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded,
> 1);
> +  if (m5LevelPagingNeeded) {
> +mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
> +PatchInstructionX86 (gPatch5LevelPagingNeeded, TRUE, 1);

1. this change assumes the default value in assembly is 0 while old logic 
doesn't
have such assumption. Can you patch the instruction no matter 
m5LevelPagingNeeded?


> +  DEBUG_CODE (
> +if (((Attributes & EFI_MEMORY_RO) == 0) || (((Attributes &
> EFI_MEMORY_XP) == 0) && (mXdSupported))) {
> +//
> +// When mapping a range to present and EFI_MEMORY_RO or
> EFI_MEMORY_XP is not specificed,
> +// check if [BaseAddress, BaseAddress + Length] contains present 
> range.
> +// Existing Present range in [BaseAddress, BaseAddress + Length] is 
> set to
> NX disable and ReadOnly.
> +//
> +Count  = 0;
> +Map= NULL;
> +Status = PageTableParse (PageTableBase, mPagingMode, NULL, );
> +while (Status == RETURN_BUFFER_TOO_SMALL) {
> +  if (Map != NULL) {
> +FreePool (Map);
> +  }
> 
> -  if (IsModified != NULL) {
> -*IsModified = TRUE;
> +  Map = AllocatePool (Count * sizeof (IA32_MAP_ENTRY));
> +  ASSERT (Map != NULL);
> +  Status = PageTableParse (PageTableBase, mPagingMode, Map, );
> +}
> +
> +ASSERT_RETURN_ERROR (Status);
> +
> +for (Index = 0; Index < Count; Index++) {
> +  if ((BaseAddress < Map[Index].LinearAddress +
> +   Map[Index].Length) && (BaseAddress + Length >
> Map[Index].LinearAddress))
> +  {
> +DEBUG ((DEBUG_ERROR, "SMM ConvertMemoryPageAttributes:
> Existing Present range in [0x%lx, 0x%lx] is set to NX disable and ReadOnly\n",
> BaseAddress, BaseAddress + Length));
> +break;
> +  }
> +}
> +
> +FreePool (Map);
>}

2. What's the purpose of the above DEBUG_CODE()?
Because when mapping a range of memory from not-present to present,
the function clears all other attributes but only set the "present" bit.
If part of the range is "present" already, the function might reset
its other attributes. This is not expected by caller.
So, you want to notify caller?

Can you split this logic to a separate commit?
If the sub-range's attributes match to what you are going to set
for the entire range, caller can ignore such error message, right?



> 
> -  //
> -  // Just split current page
> -  // Convert success in next around
> -  //
> +);
>  }
>}
> 
> +  if (PagingAttrMask.Uint64 == 0) {
> +return RETURN_SUCCESS;
> +  }
> +
> +  PageTableBufferSize = 0;
> +  Status  = PageTableMap (, PagingMode, NULL,
> , BaseAddress, Length, ,
> , IsModified);
> +
> +  if (Status == RETURN_BUFFER_TOO_SMALL) {
> +PageTableBuffer = AllocatePageTableMemory (EFI_SIZE_TO_PAGES
> (PageTableBufferSize));
> +ASSERT (PageTableBuffer != NULL);
> +Status = PageTableMap (, PagingMode, PageTableBuffer,
> , BaseAddress, Length, ,
> , IsModified);
> +  }
> +
> +  if (Status == RETURN_INVALID_PARAMETER) {
> +//
> +// The only reason that PageTableMap returns
> RETURN_INVALID_PARAMETER here is to modify other attributes
> +// of a non-present range but remains the non-present range still as non-
> present.
> +//
> +DEBUG ((DEBUG_ERROR, "SMM ConvertMemoryPageAttributes: Non-
> present range in [0x%lx, 0x%lx] needs to be removed\n", BaseAddress,
> BaseAddress + Length));

3. Don't quite understand. Can you describe in a clearer way?




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105520): https://edk2.groups.io/g/devel/message/105520
Mute This Topic: https://groups.io/mt/98922930/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] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.

[Ni, Ray]

Jiewen,
Good suggestion.
This also resolve's Jiaxin's comments that check if the variable is 
non-volatile.

Thanks,
Ray

> -Original Message-
> From: Yao, Jiewen 
> Sent: Thursday, June 1, 2023 9:06 AM
> To: devel@edk2.groups.io; Wu, Jiaxin ; Li, Zhihao
> ; Gao, Liming ; Ni, Ray
> ; kra...@redhat.com
> Cc: Wang, Jian J 
> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> add Ap rendezvous check before SmmSetVariable.
> 
> Disabling flash is a silicon behavior.
> 
> Can we do SmmWaitForAllProcessor() in FVB driver, or SPI driver ?
> 
> Thank you
> Yao, Jiewen
> 
> 
> 
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Wu,
> Jiaxin
> > Sent: Thursday, June 1, 2023 9:03 AM
> > To: devel@edk2.groups.io; Li, Zhihao ; Gao, Liming
> > ; Ni, Ray ;
> kra...@redhat.com
> > Cc: Wang, Jian J 
> > Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> add
> > Ap rendezvous check before SmmSetVariable.
> >
> > Hi All,
> >
> > I think we need this patch:
> >
> > There is a requirement that all CPU threads must in SMM for Non-Volatile
> > variable. Because the SMM will disables the flash protection. Before that, 
> > we
> > must guarantee all CPU threads are in SMM to avoid the non-smm mode
> cpus
> > modify the flash.
> >
> >
> > Zhihao,
> >
> > I think this is only needed for the Non-Volatile, I suggest as below check:
> >
> >   if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> > if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
> >   DEBUG ((DEBUG_ERROR, " SmmVariableSetVariable: Fail to wait for 
> > all
> AP
> > check in SMM!\n"));
> >   Status = EFI_ABORTED;
> >   goto EXIT;
> > }
> >   }
> >
> > Thanks,
> > Jiaxin
> >
> > > -Original Message-
> > > From: devel@edk2.groups.io  On Behalf Of Li,
> > > Zhihao
> > > Sent: Friday, May 19, 2023 4:11 PM
> > > To: Gao, Liming ; devel@edk2.groups.io; Ni,
> > > Ray ; kra...@redhat.com
> > > Cc: Wang, Jian J 
> > > Subject: 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.
> > > >
> > 

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.

[Yao, Jiewen]

Disabling flash is a silicon behavior.

Can we do SmmWaitForAllProcessor() in FVB driver, or SPI driver ?

Thank you
Yao, Jiewen



> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Wu, Jiaxin
> Sent: Thursday, June 1, 2023 9:03 AM
> To: devel@edk2.groups.io; Li, Zhihao ; Gao, Liming
> ; Ni, Ray ; kra...@redhat.com
> Cc: Wang, Jian J 
> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add
> Ap rendezvous check before SmmSetVariable.
> 
> Hi All,
> 
> I think we need this patch:
> 
> There is a requirement that all CPU threads must in SMM for Non-Volatile
> variable. Because the SMM will disables the flash protection. Before that, we
> must guarantee all CPU threads are in SMM to avoid the non-smm mode cpus
> modify the flash.
> 
> 
> Zhihao,
> 
> I think this is only needed for the Non-Volatile, I suggest as below check:
> 
>   if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
>   DEBUG ((DEBUG_ERROR, " SmmVariableSetVariable: Fail to wait for all 
> AP
> check in SMM!\n"));
>   Status = EFI_ABORTED;
>   goto EXIT;
> }
>   }
> 
> Thanks,
> Jiaxin
> 
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Li,
> > Zhihao
> > Sent: Friday, May 19, 2023 4:11 PM
> > To: Gao, Liming ; devel@edk2.groups.io; Ni,
> > Ray ; kra...@redhat.com
> > Cc: Wang, Jian J 
> > Subject: 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 

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.

[Wu, Jiaxin]

Hi All, 

I think we need this patch:

There is a requirement that all CPU threads must in SMM for Non-Volatile 
variable. Because the SMM will disables the flash protection. Before that, we 
must guarantee all CPU threads are in SMM to avoid the non-smm mode cpus modify 
the flash.


Zhihao,

I think this is only needed for the Non-Volatile, I suggest as below check:

  if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
  DEBUG ((DEBUG_ERROR, " SmmVariableSetVariable: Fail to wait for all 
AP check in SMM!\n"));
  Status = EFI_ABORTED;
  goto EXIT;
}
  }

Thanks,
Jiaxin

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Li,
> Zhihao
> Sent: Friday, May 19, 2023 4:11 PM
> To: Gao, Liming ; devel@edk2.groups.io; Ni,
> Ray ; kra...@redhat.com
> Cc: Wang, Jian J 
> Subject: 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
> 

Re: [edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX

[Ard Biesheuvel]

On Wed, 31 May 2023 at 21:04, Tom Lendacky  wrote:
>
> On 5/30/23 20:29, Ni, Ray via groups.io wrote:
> > +@Abner Chang and @Tom Lendacky
> >
> >> -Original Message-
> >> From: Tan, Dun 
> >> Sent: Tuesday, May 30, 2023 6:25 PM
> >> To: Ni, Ray ; Ard Biesheuvel ;
> >> devel@edk2.groups.io
> >> Cc: Yao, Jiewen ; Gerd Hoffmann
> >> ; Taylor Beebe ; Oliver Smith-
> >> Denny ; Bi, Dandan ; Gao,
> >> Liming ; Kinney, Michael D
> >> ; Leif Lindholm ;
> >> Sunil V L ; Warkentin, Andrei
> >> 
> >> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
> >> attribute PPI to remap the stack NX
> >>
> >> Ray,
> >> I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
> >> The only question that comes to my mind is the AMD sev feature. Since the
> >> MemoryAttribute can't handle the AMD sev feature requirements(remapping
> >> ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an
> >> appropriate place to remap the Ghcb range.
>
> I'm not sure I follow. How and where would the PPI be used? And what is
> meant by "remapping the ghcb range from non-1:1 mapping to 1:1 mapping?
>

The problem is that, for some reason, the x86 code that recreates the
page tables in permanent PEI memory is part of the DxeIpl, and
executes just before handing over to DXE core (as opposed to when
permanent PEI memory first becomes available.)

So we ended up with a highly bespoke API that creates a new set of
page tablles from scratch, with special handling of the DXE stack and
GHCB region, as they need special permissions in the page tables.

IMHO it would make more sense to
- create the new page tables as soon as PEI permanent memory becomes available
- map the GHCB region shared from a SEV specific PEIM
- map shadowed PEIMs RO as they are being dispatched
- map the PEI stack and DXE stack NX as they are allocated (or even
better, map all memory NX by default and convert to R-X as needed)

Most of these cases could make use of the new generic MemoryAttributes
PPI that I am proposing, but this requires some refactoring first to
move the pieces out of DxeIpl that are better done elsewhere.

The generic DxeIpl code that I am proposing only manages the
permissions of the DXE stack, which it allocates, and uses the PPI.
X64 should be able to reuse the same code once the above changes are
implemented.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105516): https://edk2.groups.io/g/devel/message/105516
Mute This Topic: https://groups.io/mt/99131196/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] UnitTestFrameworkPkg: UnitTestPersistenceLib: Save Unit Test Cache to Drive

[Michael D Kinney]

Would it be more flexible for unit tests apps to support an optional command 
line arg 
to specify the volume to write results? 

Mike

> -Original Message-
> From: Kun Qin 
> Sent: Wednesday, May 31, 2023 12:14 PM
> To: devel@edk2.groups.io
> Cc: Sean Brogan ; Michael Kubacki
> ; Kinney, Michael D
> 
> Subject: [PATCH v1 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: Save
> Unit Test Cache to Drive
> 
> From: kuqin12 <42554914+kuqi...@users.noreply.github.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4467
> 
> Current implementation of UnitTestFrameworkPkg for shell-based unit test
> will save the unit test cache to the same volume as the test application
> itself. This works as long as the test application is on a writable
> volume, such as USB or EFI partition.
> 
> Instead of saving the files to the same file system of unit test
> application, this change will save the cache file to the path where the
> user ran this test application.
> 
> This change was tested on proprietary physical hardware platforms and
> QEMU based virtual platform.
> 
> Cc: Sean Brogan 
> Cc: Michael Kubacki 
> Cc: Michael D Kinney 
> Signed-off-by: Kun Qin 
> ---
> 
> UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTes
> tPersistenceLibSimpleFileSystem.c | 81 +++-
>  1 file changed, 43 insertions(+), 38 deletions(-)
> 
> diff --git
> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT
> estPersistenceLibSimpleFileSystem.c
> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT
> estPersistenceLibSimpleFileSystem.c
> index b59991683f48..d4181808b2be 100644
> ---
> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT
> estPersistenceLibSimpleFileSystem.c
> +++
> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT
> estPersistenceLibSimpleFileSystem.c
> @@ -22,7 +22,7 @@
>  #define CACHE_FILE_SUFFIX  L"_Cache.dat"
> 
> 
> 
>  /**
> 
> -  Generate the device path to the cache file.
> 
> +  Generate the file name and path to the cache file.
> 
> 
> 
>@param[in]  FrameworkHandle  A pointer to the framework that is being
> persisted.
> 
> 
> 
> @@ -31,8 +31,8 @@
> 
> 
>  **/
> 
>  STATIC
> 
> -EFI_DEVICE_PATH_PROTOCOL *
> 
> -GetCacheFileDevicePath (
> 
> +CHAR16 *
> 
> +GetCacheFileName (
> 
>IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
> 
>)
> 
>  {
> 
> @@ -85,7 +85,12 @@ GetCacheFileDevicePath (
>// PathCleanUpDirectories (FileNameCopy);
> 
>// if (PathRemoveLastItem (FileNameCopy)) {
> 
>//
> 
> -  AppPath  = ConvertDevicePathToText (LoadedImage->FilePath,
> TRUE, TRUE); // NOTE: This must be freed.
> 
> +  AppPath = ConvertDevicePathToText (LoadedImage->FilePath, TRUE, TRUE);
> // NOTE: This must be freed.
> 
> +  if (AppPath == NULL) {
> 
> +DEBUG ((DEBUG_ERROR, "%a - Failed to convert device path to text.\n",
> __func__));
> 
> +goto Exit;
> 
> +  }
> 
> +
> 
>DirectorySlashOffset = StrLen (AppPath);
> 
>//
> 
>// Make sure we didn't get any weird data.
> 
> @@ -148,15 +153,15 @@ Exit:
>  FreePool (AppPath);
> 
>}
> 
> 
> 
> -  if (CacheFilePath != NULL) {
> 
> -FreePool (CacheFilePath);
> 
> +  if (CacheFileDevicePath != NULL) {
> 
> +FreePool (CacheFileDevicePath);
> 
>}
> 
> 
> 
>if (TestName != NULL) {
> 
>  FreePool (TestName);
> 
>}
> 
> 
> 
> -  return CacheFileDevicePath;
> 
> +  return CacheFilePath;
> 
>  }
> 
> 
> 
>  /**
> 
> @@ -175,21 +180,21 @@ DoesCacheExist (
>IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
> 
>)
> 
>  {
> 
> -  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
> 
> -  EFI_STATUSStatus;
> 
> -  SHELL_FILE_HANDLE FileHandle;
> 
> +  CHAR16 *FileName;
> 
> +  EFI_STATUS Status;
> 
> +  SHELL_FILE_HANDLE  FileHandle;
> 
> 
> 
>//
> 
>// NOTE: This devpath is allocated and must be freed.
> 
>//
> 
> -  FileDevicePath = GetCacheFileDevicePath (FrameworkHandle);
> 
> +  FileName = GetCacheFileName (FrameworkHandle);
> 
> 
> 
>//
> 
>// Check to see whether the file exists.  If the file can be opened for
> 
>// reading, it exists.  Otherwise, probably not.
> 
>//
> 
> -  Status = ShellOpenFileByDevicePath (
> 
> - ,
> 
> +  Status = ShellOpenFileByName (
> 
> + FileName,
> 
>   ,
> 
>   EFI_FILE_MODE_READ,
> 
>   0
> 
> @@ -198,8 +203,8 @@ DoesCacheExist (
>  ShellCloseFile ();
> 
>}
> 
> 
> 
> -  if (FileDevicePath != NULL) {
> 
> -FreePool (FileDevicePath);
> 
> +  if (FileName != NULL) {
> 
> +FreePool (FileName);
> 
>}
> 
> 
> 
>DEBUG ((DEBUG_VERBOSE, "%a - Returning %d\n", __func__, !EFI_ERROR
> (Status)));
> 
> @@ -229,10 +234,10 @@ SaveUnitTestCache (
>IN UINTN   SaveStateSize
> 
>)
> 
>  {
> 
> -  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
> 
> -  EFI_STATUS

[edk2-devel] [PATCH v1 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: Save Unit Test Cache to Drive

[Kun Qin]

From: kuqin12 <42554914+kuqi...@users.noreply.github.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4467

Current implementation of UnitTestFrameworkPkg for shell-based unit test
will save the unit test cache to the same volume as the test application
itself. This works as long as the test application is on a writable
volume, such as USB or EFI partition.

Instead of saving the files to the same file system of unit test
application, this change will save the cache file to the path where the
user ran this test application.

This change was tested on proprietary physical hardware platforms and
QEMU based virtual platform.

Cc: Sean Brogan 
Cc: Michael Kubacki 
Cc: Michael D Kinney 
Signed-off-by: Kun Qin 
---
 
UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
 | 81 +++-
 1 file changed, 43 insertions(+), 38 deletions(-)

diff --git 
a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
 
b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
index b59991683f48..d4181808b2be 100644
--- 
a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
+++ 
b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
@@ -22,7 +22,7 @@
 #define CACHE_FILE_SUFFIX  L"_Cache.dat"
 
 /**
-  Generate the device path to the cache file.
+  Generate the file name and path to the cache file.
 
   @param[in]  FrameworkHandle  A pointer to the framework that is being 
persisted.
 
@@ -31,8 +31,8 @@
 
 **/
 STATIC
-EFI_DEVICE_PATH_PROTOCOL *
-GetCacheFileDevicePath (
+CHAR16 *
+GetCacheFileName (
   IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
   )
 {
@@ -85,7 +85,12 @@ GetCacheFileDevicePath (
   // PathCleanUpDirectories (FileNameCopy);
   // if (PathRemoveLastItem (FileNameCopy)) {
   //
-  AppPath  = ConvertDevicePathToText (LoadedImage->FilePath, TRUE, 
TRUE); // NOTE: This must be freed.
+  AppPath = ConvertDevicePathToText (LoadedImage->FilePath, TRUE, TRUE);   
   // NOTE: This must be freed.
+  if (AppPath == NULL) {
+DEBUG ((DEBUG_ERROR, "%a - Failed to convert device path to text.\n", 
__func__));
+goto Exit;
+  }
+
   DirectorySlashOffset = StrLen (AppPath);
   //
   // Make sure we didn't get any weird data.
@@ -148,15 +153,15 @@ Exit:
 FreePool (AppPath);
   }
 
-  if (CacheFilePath != NULL) {
-FreePool (CacheFilePath);
+  if (CacheFileDevicePath != NULL) {
+FreePool (CacheFileDevicePath);
   }
 
   if (TestName != NULL) {
 FreePool (TestName);
   }
 
-  return CacheFileDevicePath;
+  return CacheFilePath;
 }
 
 /**
@@ -175,21 +180,21 @@ DoesCacheExist (
   IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
   )
 {
-  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
-  EFI_STATUSStatus;
-  SHELL_FILE_HANDLE FileHandle;
+  CHAR16 *FileName;
+  EFI_STATUS Status;
+  SHELL_FILE_HANDLE  FileHandle;
 
   //
   // NOTE: This devpath is allocated and must be freed.
   //
-  FileDevicePath = GetCacheFileDevicePath (FrameworkHandle);
+  FileName = GetCacheFileName (FrameworkHandle);
 
   //
   // Check to see whether the file exists.  If the file can be opened for
   // reading, it exists.  Otherwise, probably not.
   //
-  Status = ShellOpenFileByDevicePath (
- ,
+  Status = ShellOpenFileByName (
+ FileName,
  ,
  EFI_FILE_MODE_READ,
  0
@@ -198,8 +203,8 @@ DoesCacheExist (
 ShellCloseFile ();
   }
 
-  if (FileDevicePath != NULL) {
-FreePool (FileDevicePath);
+  if (FileName != NULL) {
+FreePool (FileName);
   }
 
   DEBUG ((DEBUG_VERBOSE, "%a - Returning %d\n", __func__, !EFI_ERROR 
(Status)));
@@ -229,10 +234,10 @@ SaveUnitTestCache (
   IN UINTN   SaveStateSize
   )
 {
-  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
-  EFI_STATUSStatus;
-  SHELL_FILE_HANDLE FileHandle;
-  UINTN WriteCount;
+  CHAR16 *FileName;
+  EFI_STATUS Status;
+  SHELL_FILE_HANDLE  FileHandle;
+  UINTN  WriteCount;
 
   //
   // Check the inputs for sanity.
@@ -245,13 +250,13 @@ SaveUnitTestCache (
   // Determine the path for the cache file.
   // NOTE: This devpath is allocated and must be freed.
   //
-  FileDevicePath = GetCacheFileDevicePath (FrameworkHandle);
+  FileName = GetCacheFileName (FrameworkHandle);
 
   //
   // First lets open the file if it exists so we can delete it...This is the 
work around for truncation
   //
-  Status = ShellOpenFileByDevicePath (
- ,
+  Status = ShellOpenFileByName (
+ FileName,
  ,
  (EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE),
  0
@@ -270,8 +275,8 @@ SaveUnitTestCache (
   //
   // Now that we know the 

[edk2-devel] [PATCH v1 0/1] Add support for running shell test application in an immutable volume

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4467

Current implementation of UnitTestFrameworkPkg for shell-based unit test
will save the unit test cache to the same file system as the test
application itself. This works as long as the test application is on a
writable volume, such as USB or EFI partition.

However, sometimes the test application would live inside the firmware
volume because it was integrated into the UEFI image and published
through drivers such as "FvSimpleFileSystem". In this case, attempting to
write cache file to the file system will fail and thus unable to run the
tests.

The included change will write the test cache to the path where this unit
test is invoked. i.e. test is on FS0, which is a FV file system, but FS1
is a usb file system. The traditional flow of running this test in shell
will be "fs0:", "test.efi", which will fail to write to "fs0:". The
updated flow is to navigate shell to "fs1:", then execute the test with
"fs0:test.efi". This way the cache file will be saved to "fs1:" properly.

This change will have no impact on existing users who already run shell
based tests from writeable file systesms, such as USB, FAT on NVMe.

Patch v1 branch: https://github.com/kuqin12/edk2/tree/unit_test_fv_v1.

Cc: Michael D Kinney 
Cc: Michael Kubacki 
Cc: Sean Brogan 

kuqin12 (1):
  UnitTestFrameworkPkg: UnitTestPersistenceLib: Save Unit Test Cache to
Drive

 
UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
 | 81 +++-
 1 file changed, 43 insertions(+), 38 deletions(-)

-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105513): https://edk2.groups.io/g/devel/message/105513
Mute This Topic: https://groups.io/mt/99249445/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX

[Lendacky, Thomas via groups.io]

On 5/30/23 20:29, Ni, Ray via groups.io wrote:

+@Abner Chang and @Tom Lendacky


-Original Message-
From: Tan, Dun 
Sent: Tuesday, May 30, 2023 6:25 PM
To: Ni, Ray ; Ard Biesheuvel ;
devel@edk2.groups.io
Cc: Yao, Jiewen ; Gerd Hoffmann
; Taylor Beebe ; Oliver Smith-
Denny ; Bi, Dandan ; Gao,
Liming ; Kinney, Michael D
; Leif Lindholm ;
Sunil V L ; Warkentin, Andrei

Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
attribute PPI to remap the stack NX

Ray,
I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
The only question that comes to my mind is the AMD sev feature. Since the
MemoryAttribute can't handle the AMD sev feature requirements(remapping
ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an
appropriate place to remap the Ghcb range.


I'm not sure I follow. How and where would the PPI be used? And what is 
meant by "remapping the ghcb range from non-1:1 mapping to 1:1 mapping?


Thanks,
Tom



Thanks,
Dun

-Original Message-
From: Ni, Ray 
Sent: Tuesday, May 30, 2023 3:19 PM
To: Ard Biesheuvel ; devel@edk2.groups.io; Tan, Dun

Cc: Yao, Jiewen ; Gerd Hoffmann
; Taylor Beebe ; Oliver Smith-
Denny ; Bi, Dandan ; Gao,
Liming ; Kinney, Michael D
; Leif Lindholm ;
Sunil V L ; Warkentin, Andrei

Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
attribute PPI to remap the stack NX

Looks good.

@Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what
opens will there be for X64 DxeIpl?


-Original Message-
From: Ard Biesheuvel 
Sent: Thursday, May 25, 2023 10:31 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel ; Ni, Ray ; Yao,
Jiewen ; Gerd Hoffmann ;
Taylor Beebe ; Oliver Smith-Denny
; Bi, Dandan ; Gao, Liming
; Kinney, Michael D
; Leif Lindholm
; Sunil V L ;
Warkentin, Andrei 
Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute
PPI to remap the stack NX

If the associated PCD is set to TRUE, use the memory attribute PPI to
remap the stack non-executable. This provides a generic method for
doing so, which will be used by ARM and AArch64 as well once they move
to the generic DxeIpl handoff implementation.

Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29

++--

  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf   |  5 +++-
  2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
index a0f85ebea56e6cba..22caabb02840ba88 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
@@ -2,12 +2,15 @@
Generic version of arch-specific functionality for DxeLoad.



  Copyright (c) 2006 - 2018, Intel Corporation. All rights
reserved.

+Copyright (c) 2023, Google, LLC. All rights reserved.

  SPDX-License-Identifier: BSD-2-Clause-Patent



  **/



  #include "DxeIpl.h"



+#include 

+

  /**

 Transfers control to DxeCore.



@@ -25,9 +28,10 @@ HandOffToDxeCore (
IN EFI_PEI_HOB_POINTERS  HobList

)

  {

-  VOID*BaseOfStack;

-  VOID*TopOfStack;

-  EFI_STATUS  Status;

+  VOID*BaseOfStack;

+  VOID*TopOfStack;

+  EFI_STATUS  Status;

+  EDKII_MEMORY_ATTRIBUTE_PPI  *MemoryPpi;



//

// Allocate 128KB for the Stack

@@ -35,6 +39,25 @@ HandOffToDxeCore (
BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));

ASSERT (BaseOfStack != NULL);



+  if (PcdGetBool (PcdSetNxForStack)) {

+Status = PeiServicesLocatePpi (

+   ,

+   0,

+   NULL,

+   (VOID **)

+   );

+ASSERT_EFI_ERROR (Status);

+

+Status = MemoryPpi->SetPermissions (

+  MemoryPpi,

+  (UINTN)BaseOfStack,

+  STACK_SIZE,

+  EFI_MEMORY_XP,

+  0

+  );

+ASSERT_EFI_ERROR (Status);

+  }

+

//

// Compute the top of the stack we were allocated. Pre-allocate a
UINTN

// for safety.

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 60c998be6c1bad01..7126a96d8378d1f8 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -91,6 +91,7 @@ [Ppis]
gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES

gEdkiiPeiBootInCapsuleOnDiskModePpiGuid  ## SOMETIMES_CONSUMES

gEdkiiPeiCapsuleOnDiskPpiGuid## SOMETIMES_CONSUMES #

Consumed

on firmware update boot path

+  gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES



  [Guids]

## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"

@@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize##

CONSUMES




  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]

-  

Re: [edk2-devel] [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator and Xor field names

[Michael D Kinney]

> -Original Message-
> From: Pedro Falcato 
> Sent: Wednesday, May 31, 2023 11:17 AM
> To: Kinney, Michael D 
> Cc: devel@edk2.groups.io; Gao, Liming ; Liu,
> Zhiguang ; Oliver Smith-Denny
> ; Pop, Aaron 
> Subject: Re: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> and Xor field names
> 
> On Tue, May 30, 2023 at 7:53 PM Michael D Kinney
>  wrote:
> >
> > Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
> > operator and xor in C structures to support use of these
> > include files when building with a C++ compiler.
> >
> > This patch temporarily introduces an anonymous union to add
> > Operator and Xor fields to support migration from the current
> > field names to the new field names.
> >
> > Warning 4201 is disabled for VS20xx tool chains is a temporary
> > change to allow the use of anonymous unions.
> >
> > Cc: Liming Gao 
> > Cc: Zhiguang Liu 
> > Cc: Oliver Smith-Denny 
> > Cc: Pedro Falcato 
> > Cc: Aaron Pop 
> > Signed-off-by: Michael D Kinney 
> > ---
> >  MdePkg/Include/IndustryStandard/Tpm12.h | 22 --
> >  MdePkg/Include/IndustryStandard/Tpm20.h | 25 +++--
> >  2 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
> b/MdePkg/Include/IndustryStandard/Tpm12.h
> > index 155dcc9f5f99..56e89d9d0835 100644
> > --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> > +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> > @@ -9,6 +9,14 @@
> >  #ifndef _TPM12_H_
> >  #define _TPM12_H_
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( push )
> > +#pragma warning ( disable : 4201 )
> > +#endif
> > +
> >  ///
> >  /// The start of TPM return codes
> >  ///
> > @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
> >BOOLEAN  TPMpost;
> >BOOLEAN  TPMpostLock;
> >BOOLEAN  FIPS;
> > -  BOOLEAN   operator;
> > -  BOOLEAN   enableRevokeEK;
> > +  union {
> > +BOOLEANoperator;
> > +BOOLEANOperator;
> > +  };
> 
> Do you know if this works cleanly for the other toolchains? i.e
> supported GCCs and CLANGs?
> I don't *think* there's a warning for anonymous unions beyond passing
> -pedantic + -std=c, but it'd be good to know.
> If so, we may need a pragma for this.

I did not see any issues with my local testing or with EDK II CI.

> 
> > +  BOOLEAN  enableRevokeEK;
> >BOOLEAN  nvLocked;
> >BOOLEAN  readSRKPub;
> >BOOLEAN  tpmEstablished;
> > @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
> >
> >  #pragma pack ()
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( pop )
> > +#endif
> > +
> >  #endif
> > diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
> b/MdePkg/Include/IndustryStandard/Tpm20.h
> > index 4440f3769f26..a602c0d9c289 100644
> > --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> > +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> > @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  #include 
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( push )
> > +#pragma warning ( disable : 4201 )
> > +#endif
> > +
> >  #pragma pack (1)
> >
> >  // Annex A Algorithm Constants
> > @@ -1247,7 +1255,10 @@ typedef union {
> >TPMI_AES_KEY_BITSaes;
> >TPMI_SM4_KEY_BITSSM4;
> >TPM_KEY_BITS sym;
> > -  TPMI_ALG_HASH xor;
> > +  union {
> > +TPMI_ALG_HASH  xor;
> > +TPMI_ALG_HASH  Xor;
> > +  };
> >  } TPMU_SYM_KEY_BITS;
> >
> >  // Table 123 - TPMU_SYM_MODE Union
> > @@ -1320,7 +1331,10 @@ typedef struct {
> >  // Table 136 - TPMU_SCHEME_KEYEDHASH Union
> >  typedef union {
> >TPMS_SCHEME_HMAChmac;
> > -  TPMS_SCHEME_XOR  xor;
> > +  union {
> > +TPMS_SCHEME_XOR   xor;
> > +TPMS_SCHEME_XOR   Xor;
> > +  };
> >  } TPMU_SCHEME_KEYEDHASH;
> >
> >  // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> > @@ -1809,4 +1823,11 @@ typedef struct {
> >  #define HASH_ALG_SHA512   0x0008
> >  #define HASH_ALG_SM3_256  0x0010
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( pop )
> > +#endif
> > +
> >  #endif
> > --
> > 2.40.1.windows.1
> >
> 
> All in all, this looks ok to me. Although I have to say, I'm not a
> huge fan of the naming style inconsistency introduced here (i.e Xor vs
> hmac).
> What if we made all the struct members MixedCase? Or what if we did
> something like calling them xor_ and operator_?

The more we change, the greater the potential impact to downstream use of
these structures.  I prefer to do the smallest possible change to address
the c++ reserved 

Re: [edk2-devel] [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator and Xor field names

[Pedro Falcato]

On Tue, May 30, 2023 at 7:53 PM Michael D Kinney
 wrote:
>
> Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
> operator and xor in C structures to support use of these
> include files when building with a C++ compiler.
>
> This patch temporarily introduces an anonymous union to add
> Operator and Xor fields to support migration from the current
> field names to the new field names.
>
> Warning 4201 is disabled for VS20xx tool chains is a temporary
> change to allow the use of anonymous unions.
>
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Oliver Smith-Denny 
> Cc: Pedro Falcato 
> Cc: Aaron Pop 
> Signed-off-by: Michael D Kinney 
> ---
>  MdePkg/Include/IndustryStandard/Tpm12.h | 22 --
>  MdePkg/Include/IndustryStandard/Tpm20.h | 25 +++--
>  2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h 
> b/MdePkg/Include/IndustryStandard/Tpm12.h
> index 155dcc9f5f99..56e89d9d0835 100644
> --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> @@ -9,6 +9,14 @@
>  #ifndef _TPM12_H_
>  #define _TPM12_H_
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( push )
> +#pragma warning ( disable : 4201 )
> +#endif
> +
>  ///
>  /// The start of TPM return codes
>  ///
> @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
>BOOLEAN  TPMpost;
>BOOLEAN  TPMpostLock;
>BOOLEAN  FIPS;
> -  BOOLEAN   operator;
> -  BOOLEAN   enableRevokeEK;
> +  union {
> +BOOLEANoperator;
> +BOOLEANOperator;
> +  };

Do you know if this works cleanly for the other toolchains? i.e
supported GCCs and CLANGs?
I don't *think* there's a warning for anonymous unions beyond passing
-pedantic + -std=c, but it'd be good to know.
If so, we may need a pragma for this.

> +  BOOLEAN  enableRevokeEK;
>BOOLEAN  nvLocked;
>BOOLEAN  readSRKPub;
>BOOLEAN  tpmEstablished;
> @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
>
>  #pragma pack ()
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( pop )
> +#endif
> +
>  #endif
> diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h 
> b/MdePkg/Include/IndustryStandard/Tpm20.h
> index 4440f3769f26..a602c0d9c289 100644
> --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #include 
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( push )
> +#pragma warning ( disable : 4201 )
> +#endif
> +
>  #pragma pack (1)
>
>  // Annex A Algorithm Constants
> @@ -1247,7 +1255,10 @@ typedef union {
>TPMI_AES_KEY_BITSaes;
>TPMI_SM4_KEY_BITSSM4;
>TPM_KEY_BITS sym;
> -  TPMI_ALG_HASH xor;
> +  union {
> +TPMI_ALG_HASH  xor;
> +TPMI_ALG_HASH  Xor;
> +  };
>  } TPMU_SYM_KEY_BITS;
>
>  // Table 123 - TPMU_SYM_MODE Union
> @@ -1320,7 +1331,10 @@ typedef struct {
>  // Table 136 - TPMU_SCHEME_KEYEDHASH Union
>  typedef union {
>TPMS_SCHEME_HMAChmac;
> -  TPMS_SCHEME_XOR  xor;
> +  union {
> +TPMS_SCHEME_XOR   xor;
> +TPMS_SCHEME_XOR   Xor;
> +  };
>  } TPMU_SCHEME_KEYEDHASH;
>
>  // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> @@ -1809,4 +1823,11 @@ typedef struct {
>  #define HASH_ALG_SHA512   0x0008
>  #define HASH_ALG_SM3_256  0x0010
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( pop )
> +#endif
> +
>  #endif
> --
> 2.40.1.windows.1
>

All in all, this looks ok to me. Although I have to say, I'm not a
huge fan of the naming style inconsistency introduced here (i.e Xor vs
hmac).
What if we made all the struct members MixedCase? Or what if we did
something like calling them xor_ and operator_?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105510): https://edk2.groups.io/g/devel/message/105510
Mute This Topic: https://groups.io/mt/99226543/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MinPlatform: Add CpuPageTableLib required by CpuMpPeim

[Chiu, Chasel]

Reviewed-by: Chasel Chiu 

Thanks,
Chasel



> -Original Message-
> From: Ni, Ray 
> Sent: Wednesday, May 31, 2023 2:18 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Desimone, Nathaniel L
> ; Oram, Isaac W ;
> Gao, Liming ; Dong, Eric 
> Subject: [PATCH] MinPlatform: Add CpuPageTableLib required by CpuMpPeim
> 
> The patch moves the CpuPageTableLib reference from CoreDxeLib.dsc to
> CoreCommonLib.dsc since now not only DxeMpInitLib but also CpuMpPei
> depends on it.
> 
> Signed-off-by: Ray Ni 
> Cc: Chasel Chiu 
> Cc: Nate DeSimone 
> Cc: Isaac Oram 
> Cc: Liming Gao 
> Cc: Eric Dong 
> ---
>  Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc | 1 +
>  Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc| 3 +--
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> index dfe7d836d3..300b7d7652 100644
> --- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> +++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> @@ -106,6 +106,7 @@
>LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> SmmCpuFeaturesLib|UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeatures
> Lib.inf   MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf+
> CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf#
> # Platformdiff --git
> a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
> b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
> index af30f985ca..5a2cb130b3 100644
> --- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
> +++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
> @@ -1,7 +1,7 @@
>  ## @file #  Platform description. #-# Copyright (c) 2017 - 2021, Intel
> Corporation. All rights reserved.+# Copyright (c) 2017 - 2023, Intel
> Corporation. All rights reserved. # # SPDX-License-Identifier: 
> BSD-2-Clause-
> Patent #@@ -27,7 +27,6 @@
>LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuE
> xceptionHandlerLib.inf-
> CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
> MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLi
> b.inf   TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf--
> 2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105508): https://edk2.groups.io/g/devel/message/105508
Mute This Topic: https://groups.io/mt/99238376/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] UefiCpuPkg/CpuService.c:check cpu sync mode in SmmCpuRendezvous()

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu 

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Ni, Ray
> Sent: Wednesday, May 10, 2023 4:42 PM
> To: Li, Zhihao ; devel@edk2.groups.io
> Cc: Dong, Eric 
> Subject: Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg/CpuService.c:check
> cpu sync mode in SmmCpuRendezvous()
> 
> Reviewed-by: Ray Ni 
> 
> > -Original Message-
> > From: Li, Zhihao 
> > Sent: Wednesday, May 10, 2023 4:38 PM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric ; Ni, Ray 
> > Subject: [PATCH v1 1/1] UefiCpuPkg/CpuService.c:check cpu sync mode in
> > SmmCpuRendezvous()
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4431
> >
> > In Ap relaxed mode, some SMI handlers should call SmmWaitForApArrival()
> to let
> > all ap arrive in SmmCpuRendezvous(). But in traditional mode, these SMI
> > handlers don't need to call SmmWaitForApArrival() again. So it need to be
> check
> > cpu sync mode before calling SmmWaitForApArrival().
> >
> > Cc: Eric Dong 
> > Cc: Ray Ni 
> >
> > Signed-off-by: Zhihao Li 
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 19 +--
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > index 2ebf4543c3ed..391b64e9f222 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >
> >  Implementation of SMM CPU Services Protocol.
> >
> >
> >
> > -Copyright (c) 2011 - 2022, Intel Corporation. All rights reserved.
> >
> > +Copyright (c) 2011 - 2023, Intel Corporation. All rights reserved.
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> >  **/
> >
> > @@ -421,11 +421,18 @@ SmmCpuRendezvous (
> >  goto ON_EXIT;
> >
> >}
> >
> >
> >
> > -  //
> >
> > -  // There are some APs outside SMM, Wait for all avaiable APs to arrive.
> >
> > -  //
> >
> > -  SmmWaitForApArrival ();
> >
> > -  Status = mSmmMpSyncData->AllApArrivedWithException ? EFI_SUCCESS :
> > EFI_TIMEOUT;
> >
> > +  if ((mSmmMpSyncData->EffectiveSyncMode !=
> SmmCpuSyncModeTradition)
> > && !SmmCpuFeaturesNeedConfigureMtrrs ()) {
> >
> > +//
> >
> > +// There are some APs outside SMM, Wait for all avaiable APs to arrive.
> >
> > +//
> >
> > +SmmWaitForApArrival ();
> >
> > +Status = mSmmMpSyncData->AllApArrivedWithException ?
> EFI_SUCCESS :
> > EFI_TIMEOUT;
> >
> > +  } else {
> >
> > +//
> >
> > +// BSP has already waitted for APs to arrive SMM if SmmCpuSyncMode
> > selected or need config MTRR.
> >
> > +//
> >
> > +Status = EFI_TIMEOUT;
> >
> > +  }
> >
> >
> >
> >  ON_EXIT:
> >
> >if (!mSmmMpSyncData->AllApArrivedWithException) {
> >
> > --
> > 2.26.2.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105507): https://edk2.groups.io/g/devel/message/105507
Mute This Topic: https://groups.io/mt/98801800/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-redfish-client][PATCH 2/2] RedfishClientPkg: Use DEBUG_MANAGEABILITY

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Merged, thanks!

> -Original Message-
> From: Nickle Wang 
> Sent: Wednesday, May 31, 2023 8:39 PM
> To: Chang, Abner ; devel@edk2.groups.io
> Cc: Igor Kulchytskyy 
> Subject: RE: [edk2-redfish-client][PATCH 2/2] RedfishClientPkg: Use
> DEBUG_MANAGEABILITY
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Reviewed-by: Nickle Wang 
>
> Regards,
> Nickle
>
> > -Original Message-
> > From: abner.ch...@amd.com 
> > Sent: Tuesday, May 30, 2023 3:42 PM
> > To: devel@edk2.groups.io
> > Cc: Nickle Wang ; Igor Kulchytskyy 
> > Subject: [edk2-redfish-client][PATCH 2/2] RedfishClientPkg: Use
> > DEBUG_MANAGEABILITY
> >
> > External email: Use caution opening links or attachments
> >
> >
> > From: Abner Chang 
> >
> > Use debug print level DEBUG_MANAGEABILITY in
> > RedfishClientPkg.
> >
> > Signed-off-by: Abner Chang 
> > Cc: Nickle Wang 
> > Cc: Igor Kulchytskyy 
> > ---
> >  RedfishClientPkg/Include/RedfishBase.h|  2 +-
> >  .../Features/Bios/v1_0_9/Common/BiosCommon.c  | 10 
> >  .../Features/Bios/v1_0_9/Dxe/BiosDxe.c|  2 +-
> >  .../v1_5_0/Common/ComputerSystemCommon.c  |  8 +++
> >  .../v1_5_0/Dxe/ComputerSystemDxe.c|  2 +-
> >  .../ComputerSystemCollectionDxe.c |  2 +-
> >  .../Memory/V1_7_1/Common/MemoryCommon.c   |  8 +++
> >  .../Features/Memory/V1_7_1/Dxe/MemoryDxe.c|  2 +-
> >  .../MemoryCollectionDxe/MemoryCollectionDxe.c |  2 +-
> >  .../EdkIIRedfishResourceConfigLib.c   |  2 +-
> >  .../RedfishFeatureUtilityLib.c| 24 +--
> >  .../PrivateLibrary/RedfishLib/RedfishLib.c|  8 +++
> >  .../RedfishLib/edk2libredfish/src/service.c   | 18 +++---
> >  .../RedfishConfigLangMapDxe.c |  8 +++
> >  .../RedfishETagDxe/RedfishETagDxe.c   |  8 +++
> >  15 files changed, 53 insertions(+), 53 deletions(-)
> >
> > diff --git a/RedfishClientPkg/Include/RedfishBase.h
> > b/RedfishClientPkg/Include/RedfishBase.h
> > index 1abe9517ef..d58d7579ce 100644
> > --- a/RedfishClientPkg/Include/RedfishBase.h
> > +++ b/RedfishClientPkg/Include/RedfishBase.h
> > @@ -11,7 +11,7 @@
> >  #define EFI_REDFISH_BASE_H_
> >
> >  #define IS_EMPTY_STRING(a)  ((a) == NULL || (a)[0] == '\0')
> > -#define REDFISH_DEBUG_TRACE  DEBUG_INFO
> > +#define REDFISH_DEBUG_TRACE  DEBUG_MANAGEABILITY
> >
> >  ///
> >  /// This GUID is used for an EFI Variable that stores the Redfish data.
> > diff --git
> a/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> > b/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> > index 82a3d46548..8442608813 100644
> > --- a/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> > +++ b/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> > @@ -65,7 +65,7 @@ RedfishConsumeResourceCommon (
> >  //
> >  // No change
> >  //
> > -DEBUG ((DEBUG_INFO, "%a, ETAG: %s has no change, ignore consume
> > action\n", __func__, Private->Uri));
> > +DEBUG ((DEBUG_MANAGEABILITY, "%a, ETAG: %s has no change, ignore
> > consume action\n", __func__, Private->Uri));
> >  Status = EFI_ALREADY_STARTED;
> >  goto ON_RELEASE;
> >}
> > @@ -352,7 +352,7 @@ ProvisioningBiosResources (
> >EdkIIRedfishResourceSetConfigureLang ();
> >
> >for (Index = 0; Index < UnifiedConfigureLangList.Count; Index++) {
> > -DEBUG ((DEBUG_INFO, "[%d] create Bios resource from: %s\n",
> > UnifiedConfigureLangList.List[Index].Index,
> > UnifiedConfigureLangList.List[Index].ConfigureLang));
> > +DEBUG ((DEBUG_MANAGEABILITY, "[%d] create Bios resource from:
> %s\n",
> > UnifiedConfigureLangList.List[Index].Index,
> > UnifiedConfigureLangList.List[Index].ConfigureLang));
> >  ProvisioningBiosResource (Private,
> UnifiedConfigureLangList.List[Index].Index,
> > UnifiedConfigureLangList.List[Index].ConfigureLang);
> >  FreePool (UnifiedConfigureLangList.List[Index].ConfigureLang);
> >}
> > @@ -498,9 +498,9 @@ RedfishCheckResourceCommon (
> >continue;
> >  }
> >
> > -DEBUG ((DEBUG_INFO, "%a, [%d] check attribute for: %s\n", __func__,
> Index,
> > Property));
> > +DEBUG ((DEBUG_MANAGEABILITY, "%a, [%d] check attribute for: %s\n",
> > __func__, Index, Property));
> >  if (!MatchPropertyWithJsonContext (Property, Json)) {
> > -  DEBUG ((DEBUG_INFO, "%a, property is missing: %s\n", __func__,
> Property));
> > +  DEBUG ((DEBUG_MANAGEABILITY, "%a, property is missing: %s\n",
> > __func__, Property));
> >Status = EFI_NOT_FOUND;
> >  }
> >}
> > @@ -692,7 +692,7 @@ HandleResource (
> >  Status = EdkIIRedfishResourceConfigIdentify (, Uri, Private-
> > >InformationExchange);
> >  if (EFI_ERROR (Status)) {
> >if (Status == EFI_UNSUPPORTED) {
> > -DEBUG ((DEBUG_INFO, "%a, \"%s\" is not handled by us\n", __func__,
> Uri));
> > +

Re: [edk2-devel] [PATCH v2 1/1] UefiCpuPkg/PiSmmCpuDxeSmm:add Ap Rendezvous check in PerformRemainingTasks.

[Wu, Jiaxin]

> 
> +if (EFI_ERROR (SmmCpuRendezvous (NULL, TRUE))) {
> 
> +  DEBUG ((DEBUG_ERROR, "PerformRemainingTasks: fail to wait for all AP
> check in SMM!\n"));

If error happen, do we should return directly or continue the executing?

Thanks,
Jiaxin 

> 
> +}
> 
> +
> 
>  //
> 
>  // Start SMM Profile feature
> 
>  //
> 
> --
> 2.26.2.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#103972):
> https://edk2.groups.io/g/devel/message/103972
> Mute This Topic: https://groups.io/mt/98679265/1787330
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiaxin...@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105505): https://edk2.groups.io/g/devel/message/105505
Mute This Topic: https://groups.io/mt/98679265/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] ShellPkg\SmbiosView: SmBiosView does not print correct Slot ID information

[Adrian Sperber via groups.io]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4469

SlotType related check only supports PcieGen3 checks within
DisplaySystemSlotId.
SmbiosView will print:
Slot Id: undefined Slot Id
Updating this check to Gen6andBeyond which fixes the problem:
Slot Id: the value present in the Slot Number field of the PCI Interrupt
Routing table entry that is associated with this slot is: 

Signed-off-by: Adrian Sperber 
---
 .../Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
index a14b79904d..b109934f58 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
@@ -2998,7 +2998,7 @@ DisplaySystemSlotId (
   break;

 default:
-  if (((SlotType >= 0x0E) && (SlotType <= 0x12)) || ((SlotType >= 0xA6) && 
(SlotType <= 0xB6))) {
+  if (((SlotType >= 0x0E) && (SlotType <= 0x12)) || ((SlotType >= 0xA6) && 
(SlotType <= SlotTypePCIExpressGen6andBeyond))) {
 ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN 
(STR_SMBIOSVIEW_PRINTINFO_VALUE_PRESENT), gShellDebug1HiiHandle, SlotId);
   } else {
 ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN 
(STR_SMBIOSVIEW_PRINTINFO_UNDEFINED_SLOT_ID), gShellDebug1HiiHandle);
--
2.38.1.windows.1


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for 
the sole use of the intended recipient(s) and contains information that is 
confidential and proprietary to Ampere Computing or its subsidiaries. It is to 
be used solely for the purpose of furthering the parties' business 
relationship. Any unauthorized review, copying, or distribution of this email 
(or any attachments thereto) is strictly prohibited. If you are not the 
intended recipient, please contact the sender immediately and permanently 
delete the original and any copies of this email and any attachments thereto.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105503): https://edk2.groups.io/g/devel/message/105503
Mute This Topic: https://groups.io/mt/99244489/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] IntelFsp2Pkg/Tools: PatchFv: Handle 32-bit address in GCC map

[Jiaqing Zhao]

Some versions of ld (like 2.40 in Ubuntu 23.04) uses 32-bit address
when generating map files for IA32 build. This patch enables PatchFv.py
to parse these 32-bit addresses in GCC map properly.

Signed-off-by: Jiaqing Zhao 
---
 IntelFsp2Pkg/Tools/PatchFv.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/IntelFsp2Pkg/Tools/PatchFv.py b/IntelFsp2Pkg/Tools/PatchFv.py
index 73ab877c71..2e60343180 100644
--- a/IntelFsp2Pkg/Tools/PatchFv.py
+++ b/IntelFsp2Pkg/Tools/PatchFv.py
@@ -432,7 +432,8 @@ class Symbols:
 if reportLine.strip().find("Archive member included") != -1:
 #GCC
 #0x1d55IoRead8
-patchMapFileMatchString = 
"\s+(0x[0-9a-fA-F]{16})\s+([^\s][^0x][_a-zA-Z0-9\-]+)\s"
+#0x1d55IoRead8
+patchMapFileMatchString = 
"\s+(0x[0-9a-fA-F]{8,16})\s+([^\s][^0x][_a-zA-Z0-9\-]+)\s"
 matchKeyGroupIndex = 2
 matchSymbolGroupIndex  = 1
 prefix = '_'
@@ -458,7 +459,7 @@ class Symbols:
 if handleNext:
 handleNext = False
 pcdName = match.group(1)
-match   = re.match("\s+(0x[0-9a-fA-F]{16})\s+", reportLine)
+match   = re.match("\s+(0x[0-9a-fA-F]{8,16})\s+", 
reportLine)
 if match is not None:
 modSymbols[prefix + pcdName] = match.group(1)
 else:
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105504): https://edk2.groups.io/g/devel/message/105504
Mute This Topic: https://groups.io/mt/99244490/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg: Fix port multiplier port in AhciPei PEIM

[He, Jiangang via groups.io]

[AMD Official Use Only - General]

We did crisis recovery and Opal HD password unlock from S3 resume from SATA HD 
test on two different version of AHCI host controllers.

Thanks,
Jiangang
-Original Message-
From: Wu, Hao A 
Sent: Tuesday, May 30, 2023 10:30 PM
To: devel@edk2.groups.io; Hsueh, Hong-Chih (Neo) 
Cc: He, Jiangang ; Chang, Abner 
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: Fix port multiplier port in 
AhciPei PEIM

Caution: This message originated from an External Source. Use proper caution 
when opening attachments, clicking links, or responding.


Thanks, the code changes look good to me.
May I know what tests have been performed for the patch?

Best Regards,
Hao Wu

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Neo
> Hsueh via groups.io
> Sent: Wednesday, May 24, 2023 1:07 AM
> To: devel@edk2.groups.io
> Cc: jiangang...@amd.com; abner.ch...@amd.com; Neo Hsueh  chih.hs...@amd.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg: Fix port multiplier port
> in AhciPei PEIM
>
> If there is no port multiplier, PortMultiplierPort should be converted
> to 0 to follow AHCI spec.
> The same logic already applied in AtaAtapiPassThruDxe driver.
>
> Signed-off-by: Neo Hsueh 
> ---
>  MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
> b/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
> index cd55272c96..7bd04661d0 100644
> --- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
> +++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c
> @@ -3,6 +3,7 @@
>mode at PEI phase.
>
>Copyright (c) 2019, Intel Corporation. All rights reserved.
> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> + reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -92,6 +93,15 @@ AhciPassThruExecute (  {
>EFI_STATUS  Status;
>
> +  if (PortMultiplierPort == 0x) {
> +//
> +// If there is no port multiplier, PortMultiplierPort will be 0x
> +// according to UEFI spec. Here, we convert its value to 0 to follow
> +// AHCI spec.
> +//
> +PortMultiplierPort = 0;
> +  }
> +
>switch (Packet->Protocol) {
>  case EFI_ATA_PASS_THRU_PROTOCOL_ATA_NON_DATA:
>Status = AhciNonDataTransfer (
> --
> 2.40.0.windows.1
>
>
>
> 
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105502): https://edk2.groups.io/g/devel/message/105502
Mute This Topic: https://groups.io/mt/99093106/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Event: TianoCore edk2-test Bug Triage Meeting - Thursday, June 1, 2023 #cal-reminder

[Group Notification]

*Reminder: TianoCore edk2-test Bug Triage Meeting*

*When:*
Thursday, June 1, 2023
10:00pm to 11:00pm
(UTC+08:00) Asia/Shanghai

*Where:*
https://armltd.zoom.us/j/91247522013?pwd=ei9nUndTbG9oWEROS2M1aVREZkpiQT09=addon

*Organizer:* Edhaya Chandran edhaya.chand...@arm.com ( 
edhaya.chand...@arm.com?subject=Re:%20Event:%20TianoCore%20edk2-test%20Bug%20Triage%20Meeting
 )

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=1890538 )

*Description:*


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105501): https://edk2.groups.io/g/devel/message/105501
Mute This Topic: https://groups.io/mt/99242684/21656
Mute #cal-reminder:https://edk2.groups.io/g/devel/mutehashtag/cal-reminder
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-redfish-client][PATCH 2/2] RedfishClientPkg: Use DEBUG_MANAGEABILITY

[Nickle Wang via groups.io]

Reviewed-by: Nickle Wang 

Regards,
Nickle

> -Original Message-
> From: abner.ch...@amd.com 
> Sent: Tuesday, May 30, 2023 3:42 PM
> To: devel@edk2.groups.io
> Cc: Nickle Wang ; Igor Kulchytskyy 
> Subject: [edk2-redfish-client][PATCH 2/2] RedfishClientPkg: Use
> DEBUG_MANAGEABILITY
> 
> External email: Use caution opening links or attachments
> 
> 
> From: Abner Chang 
> 
> Use debug print level DEBUG_MANAGEABILITY in
> RedfishClientPkg.
> 
> Signed-off-by: Abner Chang 
> Cc: Nickle Wang 
> Cc: Igor Kulchytskyy 
> ---
>  RedfishClientPkg/Include/RedfishBase.h|  2 +-
>  .../Features/Bios/v1_0_9/Common/BiosCommon.c  | 10 
>  .../Features/Bios/v1_0_9/Dxe/BiosDxe.c|  2 +-
>  .../v1_5_0/Common/ComputerSystemCommon.c  |  8 +++
>  .../v1_5_0/Dxe/ComputerSystemDxe.c|  2 +-
>  .../ComputerSystemCollectionDxe.c |  2 +-
>  .../Memory/V1_7_1/Common/MemoryCommon.c   |  8 +++
>  .../Features/Memory/V1_7_1/Dxe/MemoryDxe.c|  2 +-
>  .../MemoryCollectionDxe/MemoryCollectionDxe.c |  2 +-
>  .../EdkIIRedfishResourceConfigLib.c   |  2 +-
>  .../RedfishFeatureUtilityLib.c| 24 +--
>  .../PrivateLibrary/RedfishLib/RedfishLib.c|  8 +++
>  .../RedfishLib/edk2libredfish/src/service.c   | 18 +++---
>  .../RedfishConfigLangMapDxe.c |  8 +++
>  .../RedfishETagDxe/RedfishETagDxe.c   |  8 +++
>  15 files changed, 53 insertions(+), 53 deletions(-)
> 
> diff --git a/RedfishClientPkg/Include/RedfishBase.h
> b/RedfishClientPkg/Include/RedfishBase.h
> index 1abe9517ef..d58d7579ce 100644
> --- a/RedfishClientPkg/Include/RedfishBase.h
> +++ b/RedfishClientPkg/Include/RedfishBase.h
> @@ -11,7 +11,7 @@
>  #define EFI_REDFISH_BASE_H_
> 
>  #define IS_EMPTY_STRING(a)  ((a) == NULL || (a)[0] == '\0')
> -#define REDFISH_DEBUG_TRACE  DEBUG_INFO
> +#define REDFISH_DEBUG_TRACE  DEBUG_MANAGEABILITY
> 
>  ///
>  /// This GUID is used for an EFI Variable that stores the Redfish data.
> diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> b/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> index 82a3d46548..8442608813 100644
> --- a/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> +++ b/RedfishClientPkg/Features/Bios/v1_0_9/Common/BiosCommon.c
> @@ -65,7 +65,7 @@ RedfishConsumeResourceCommon (
>  //
>  // No change
>  //
> -DEBUG ((DEBUG_INFO, "%a, ETAG: %s has no change, ignore consume
> action\n", __func__, Private->Uri));
> +DEBUG ((DEBUG_MANAGEABILITY, "%a, ETAG: %s has no change, ignore
> consume action\n", __func__, Private->Uri));
>  Status = EFI_ALREADY_STARTED;
>  goto ON_RELEASE;
>}
> @@ -352,7 +352,7 @@ ProvisioningBiosResources (
>EdkIIRedfishResourceSetConfigureLang ();
> 
>for (Index = 0; Index < UnifiedConfigureLangList.Count; Index++) {
> -DEBUG ((DEBUG_INFO, "[%d] create Bios resource from: %s\n",
> UnifiedConfigureLangList.List[Index].Index,
> UnifiedConfigureLangList.List[Index].ConfigureLang));
> +DEBUG ((DEBUG_MANAGEABILITY, "[%d] create Bios resource from: %s\n",
> UnifiedConfigureLangList.List[Index].Index,
> UnifiedConfigureLangList.List[Index].ConfigureLang));
>  ProvisioningBiosResource (Private, 
> UnifiedConfigureLangList.List[Index].Index,
> UnifiedConfigureLangList.List[Index].ConfigureLang);
>  FreePool (UnifiedConfigureLangList.List[Index].ConfigureLang);
>}
> @@ -498,9 +498,9 @@ RedfishCheckResourceCommon (
>continue;
>  }
> 
> -DEBUG ((DEBUG_INFO, "%a, [%d] check attribute for: %s\n", __func__, 
> Index,
> Property));
> +DEBUG ((DEBUG_MANAGEABILITY, "%a, [%d] check attribute for: %s\n",
> __func__, Index, Property));
>  if (!MatchPropertyWithJsonContext (Property, Json)) {
> -  DEBUG ((DEBUG_INFO, "%a, property is missing: %s\n", __func__, 
> Property));
> +  DEBUG ((DEBUG_MANAGEABILITY, "%a, property is missing: %s\n",
> __func__, Property));
>Status = EFI_NOT_FOUND;
>  }
>}
> @@ -692,7 +692,7 @@ HandleResource (
>  Status = EdkIIRedfishResourceConfigIdentify (, Uri, Private-
> >InformationExchange);
>  if (EFI_ERROR (Status)) {
>if (Status == EFI_UNSUPPORTED) {
> -DEBUG ((DEBUG_INFO, "%a, \"%s\" is not handled by us\n", __func__, 
> Uri));
> +DEBUG ((DEBUG_MANAGEABILITY, "%a, \"%s\" is not handled by us\n",
> __func__, Uri));
>  return EFI_SUCCESS;
>}
> 
> diff --git a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> index bbfbe7a873..623a9af8f6 100644
> --- a/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> +++ b/RedfishClientPkg/Features/Bios/v1_0_9/Dxe/BiosDxe.c
> @@ -46,7 +46,7 @@ RedfishResourceProvisioningResource (
>  return EFI_INVALID_PARAMETER;
>}
> 
> -  DEBUG ((DEBUG_INFO, "%a, provisioning in %s mode\n", __func__,
> (PostMode ? L"POST" : L"PATCH")));
> +  DEBUG 

[edk2-devel] [PATCH V2 6/6] MdeModulePkg/SmmCorePerformanceLib: Disable perf-logging at runtime

[Ni, Ray]

Because SMM perf-logging is migrated to non-SMRAM at ReadyToBoot
by DxeCorePerformanceLib, the perf-logging after ExitBS is useless and
impact the SMI latency at runtime.
Hence the SmmCorePerformanceLib is updated to disable perf-logging
after ExitBS.

Cc: Jiaxin Wu 
Cc: Dandan Bi 
Cc: Liming Gao 
Cc: Jian J Wang 
---
 .../SmmCorePerformanceLib.c   | 48 ++-
 .../SmmCorePerformanceLib.inf |  3 +-
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c 
b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
index 3efe56e056..c566a298dd 100644
--- a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
+++ b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
@@ -16,7 +16,7 @@
 
  SmmPerformanceHandlerEx(), SmmPerformanceHandler() will receive untrusted 
input and do basic validation.
 
-Copyright (c) 2011 - 2021, Intel Corporation. All rights reserved.
+Copyright (c) 2011 - 2023, Intel Corporation. All rights reserved.
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -48,6 +48,7 @@ SPIN_LOCK mSmmFpdtLock;
 PERFORMANCE_PROPERTY  mPerformanceProperty;
 UINT32mCachedLength   = 0;
 UINT32mBootRecordSize = 0;
+BOOLEAN   mPerformanceMeasurementEnabled;
 
 //
 // Interfaces for SMM PerformanceMeasurement Protocol.
@@ -929,6 +930,36 @@ FpdtSmiHandler (
   return EFI_SUCCESS;
 }
 
+/**
+  This is the Event call back function is triggered in SMM to notify the 
Library
+  the system is entering runtime phase.
+
+  @param[in] Protocol   Points to the protocol's unique identifier
+  @param[in] Interface  Points to the interface instance
+  @param[in] Handle The handle on which the interface was installed
+
+  @retval EFI_SUCCESS SmmAtRuntimeCallBack runs successfully
+ **/
+EFI_STATUS
+EFIAPI
+SmmCorePerformanceLibExitBootServicesCallback (
+  IN CONST EFI_GUID  *Protocol,
+  IN VOID*Interface,
+  IN EFI_HANDLE  Handle
+  )
+{
+  //
+  // Disable performance measurement after ExitBootServices because
+  // 1. Performance measurement might impact SMI latency at runtime;
+  // 2. Performance log is copied to non SMRAM at ReadyToBoot so runtime 
performance
+  //log is not useful.
+  //
+  mPerformanceMeasurementEnabled = FALSE;
+
+  return EFI_SUCCESS;
+}
+
+
 /**
   SmmBase2 protocol notify callback function, when SMST and SMM memory service 
get initialized
   this function is callbacked to initialize the Smm Performance Lib
@@ -948,6 +979,7 @@ InitializeSmmCorePerformanceLib (
   EFI_HANDLESmiHandle;
   EFI_STATUSStatus;
   PERFORMANCE_PROPERTY  *PerformanceProperty;
+  VOID  *Registration;
 
   //
   // Initialize spin lock
@@ -987,6 +1019,16 @@ InitializeSmmCorePerformanceLib (
 Status = gBS->InstallConfigurationTable (, 
);
 ASSERT_EFI_ERROR (Status);
   }
+
+  //
+  // Register callback function for ExitBootServices event.
+  //
+  Status = gSmst->SmmRegisterProtocolNotify (
+,
+SmmCorePerformanceLibExitBootServicesCallback,
+
+);
+  ASSERT_EFI_ERROR (Status);
 }
 
 /**
@@ -1011,6 +1053,8 @@ SmmCorePerformanceLibConstructor (
   EFI_EVENT   Event;
   VOID*Registration;
 
+  mPerformanceMeasurementEnabled =  (BOOLEAN)((PcdGet8 
(PcdPerformanceLibraryPropertyMask) & 
PERFORMANCE_LIBRARY_PROPERTY_MEASUREMENT_ENABLED) != 0);
+
   if (!PerformanceMeasurementEnabled ()) {
 //
 // Do not initialize performance infrastructure if not required.
@@ -1383,7 +1427,7 @@ PerformanceMeasurementEnabled (
   VOID
   )
 {
-  return (BOOLEAN)((PcdGet8 (PcdPerformanceLibraryPropertyMask) & 
PERFORMANCE_LIBRARY_PROPERTY_MEASUREMENT_ENABLED) != 0);
+  return mPerformanceMeasurementEnabled;
 }
 
 /**
diff --git 
a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.inf 
b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.inf
index 9eecc4b58c..9a7e14e80c 100644
--- a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.inf
+++ b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.inf
@@ -8,7 +8,7 @@
 #  This library is mainly used by SMM Core to start performance logging to 
ensure that
 #  SMM Performance and PerformanceEx Protocol are installed at the very 
beginning of SMM phase.
 #
-#  Copyright (c) 2011 - 2021, Intel Corporation. All rights reserved.
+#  Copyright (c) 2011 - 2023, Intel Corporation. All rights reserved.
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -58,6 +58,7 @@
 
 [Protocols]
   gEfiSmmBase2ProtocolGuid  ## CONSUMES
+  gEdkiiSmmExitBootServicesProtocolGuid ## CONSUMES
 
 [Guids]
   ## PRODUCES ## SystemTable
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online 

[edk2-devel] [PATCH V2 5/6] MdeModulePkg/SmmPerformanceLib: Disable perf-logging after ExitBS

[Ni, Ray]

Because SMM perf-logging is migrated to non-SMRAM at ReadyToBoot
by DxeCorePerformanceLib, the perf-logging after ExitBS is useless and
impact the SMI latency at runtime.
Hence the SmmPerformanceLib is updated to disable perf-logging
after ExitBS.

Cc: Jiaxin Wu 
Cc: Dandan Bi 
Cc: Liming Gao 
Cc: Jian J Wang 
---
 .../SmmPerformanceLib/SmmPerformanceLib.c | 63 ++-
 .../SmmPerformanceLib/SmmPerformanceLib.inf   |  4 ++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.c 
b/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.c
index 623f8a978c..b9c33c0f64 100644
--- a/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.c
+++ b/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.c
@@ -23,6 +23,36 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 // The cached SMM Performance Protocol and SMM PerformanceEx Protocol 
interface.
 EDKII_PERFORMANCE_MEASUREMENT_PROTOCOL  *mPerformanceMeasurement = NULL;
 BOOLEAN mPerformanceMeasurementEnabled;
+VOID
*mPerformanceLibExitBootServicesRegistration;
+
+/**
+  This is the Event call back function is triggered in SMM to notify the 
Library
+  the system is entering runtime phase.
+
+  @param[in] Protocol   Points to the protocol's unique identifier
+  @param[in] Interface  Points to the interface instance
+  @param[in] Handle The handle on which the interface was installed
+
+  @retval EFI_SUCCESS SmmAtRuntimeCallBack runs successfully
+ **/
+EFI_STATUS
+EFIAPI
+SmmPerformanceLibExitBootServicesCallback (
+  IN CONST EFI_GUID  *Protocol,
+  IN VOID*Interface,
+  IN EFI_HANDLE  Handle
+  )
+{
+  //
+  // Disable performance measurement after ExitBootServices because
+  // 1. Performance measurement might impact SMI latency at runtime;
+  // 2. Performance log is copied to non SMRAM at ReadyToBoot so runtime 
performance
+  //log is not useful.
+  //
+  mPerformanceMeasurementEnabled = FALSE;
+
+  return EFI_SUCCESS;
+}
 
 /**
   The constructor function initializes the Performance Measurement Enable flag
@@ -40,9 +70,40 @@ SmmPerformanceLibConstructor (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  EFI_STATUS  Status;
+
   mPerformanceMeasurementEnabled =  (BOOLEAN)((PcdGet8 
(PcdPerformanceLibraryPropertyMask) & 
PERFORMANCE_LIBRARY_PROPERTY_MEASUREMENT_ENABLED) != 0);
 
-  return EFI_SUCCESS;
+  Status = gSmst->SmmRegisterProtocolNotify (
+,
+SmmPerformanceLibExitBootServicesCallback,
+
+);
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
+}
+
+EFI_STATUS
+EFIAPI
+SmmPerformanceLibDestructor (
+  IN EFI_HANDLEImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  //
+  // Unregister SmmExitBootServices notification.
+  //
+  Status = gSmst->SmmRegisterProtocolNotify (
+,
+NULL,
+
+);
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
 }
 
 /**
diff --git a/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.inf 
b/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.inf
index d79cd5c8da..002462f5ca 100644
--- a/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.inf
+++ b/MdeModulePkg/Library/SmmPerformanceLib/SmmPerformanceLib.inf
@@ -21,6 +21,7 @@
   LIBRARY_CLASS  = PerformanceLib|DXE_SMM_DRIVER
 
   CONSTRUCTOR= SmmPerformanceLibConstructor
+  DESTRUCTOR = SmmPerformanceLibDestructor
 
 #
 # The following information is for reference only and not required by the 
build tools.
@@ -46,5 +47,8 @@
 [Guids]
   gEdkiiSmmPerformanceMeasurementProtocolGuid  ## SOMETIMES_CONSUMES   
## UNDEFINED # Locate protocol
 
+[Protocols]
+  gEdkiiSmmExitBootServicesProtocolGuid## CONSUMES
+
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask## CONSUMES
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105497): https://edk2.groups.io/g/devel/message/105497
Mute This Topic: https://groups.io/mt/99240113/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V2 4/6] MdeModulePkg/SmmCore: Add perf-logging for SmmDriverDispatchHandler

[Ni, Ray]

SmmDriverDispatchHandler is the routine that dispatches SMM drivers
from FV. It's a time-consuming routine.
Add perf-logging for this routine.

Signed-off-by: Ray Ni 
Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Jiaxin Wu 
---
 MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c 
b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
index f635565dd1..bb789e5890 100644
--- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
+++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
@@ -28,7 +28,7 @@
   Depex - Dependency Expression.
 
   Copyright (c) 2014, Hewlett-Packard Development Company, L.P.
-  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
+  Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -1322,6 +1322,8 @@ SmmDriverDispatchHandler (
 return EFI_NOT_FOUND;
   }
 
+  PERF_CALLBACK_BEGIN ();
+
   for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
 FvHandle = HandleBuffer[HandleIndex];
 
@@ -1511,6 +1513,7 @@ SmmDriverDispatchHandler (
 }
   }
 
+  PERF_CALLBACK_END ();
   return EFI_SUCCESS;
 }
 
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105496): https://edk2.groups.io/g/devel/message/105496
Mute This Topic: https://groups.io/mt/99240111/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V2 3/6] MdeModulePkg/SmmCore: Add perf-logging for time-consuming procedures

[Ni, Ray]

Following procedures are perf-logged:
* SmmReadyToBootHandler
* SmmReadyToLockHandler
* SmmEndOfDxeHandler
* SmmEntryPoint
  (It's the main routine run in BSP when SMI happens.)
* SmiManage

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Jiaxin Wu 
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 14 +-
 MdeModulePkg/Core/PiSmmCore/Smi.c   |  6 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c 
b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 875c7c0258..a15afa8dd6 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -1,7 +1,7 @@
 /** @file
   SMM Core Main Entry Point
 
-  Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.
+  Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -304,6 +304,7 @@ SmmReadyToBootHandler (
 {
   EFI_STATUS  Status;
   EFI_HANDLE  SmmHandle;
+  PERF_CALLBACK_BEGIN ();
 
   //
   // Install SMM Ready To Boot protocol.
@@ -318,6 +319,7 @@ SmmReadyToBootHandler (
 
   SmiHandlerUnRegister (DispatchHandle);
 
+  PERF_CALLBACK_END ();
   return Status;
 }
 
@@ -352,6 +354,8 @@ SmmReadyToLockHandler (
   EFI_HANDLE  SmmHandle;
   VOID*Interface;
 
+  PERF_CALLBACK_BEGIN ();
+
   //
   // Unregister SMI Handlers that are no required after the SMM driver 
dispatch is stopped
   //
@@ -408,6 +412,7 @@ SmmReadyToLockHandler (
 
   SmramProfileReadyToLock ();
 
+  PERF_CALLBACK_END ();
   return Status;
 }
 
@@ -442,6 +447,8 @@ SmmEndOfDxeHandler (
 
   DEBUG ((DEBUG_INFO, "SmmEndOfDxeHandler\n"));
 
+  PERF_CALLBACK_BEGIN ();
+
   //
   // Install SMM EndOfDxe protocol
   //
@@ -479,6 +486,7 @@ SmmEndOfDxeHandler (
 }
   }
 
+  PERF_CALLBACK_END ();
   return EFI_SUCCESS;
 }
 
@@ -669,6 +677,8 @@ SmmEntryPoint (
   VOID*CommunicationBuffer;
   UINTN   BufferSize;
 
+  PERF_FUNCTION_BEGIN ();
+
   //
   // Update SMST with contents of the SmmEntryContext structure
   //
@@ -769,6 +779,8 @@ SmmEntryPoint (
 //
 gSmmCorePrivate->InSmm = FALSE;
   }
+
+  PERF_FUNCTION_END ();
 }
 
 /**
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c 
b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 6d13969979..2985f989c3 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -109,6 +109,8 @@ SmiManage (
   BOOLEAN  SuccessReturn;
   EFI_STATUS   Status;
 
+  PERF_FUNCTION_BEGIN ();
+
   Status= EFI_NOT_FOUND;
   SuccessReturn = FALSE;
   if (HandlerType == NULL) {
@@ -125,6 +127,7 @@ SmiManage (
   //
   // There is no handler registered for this interrupt source
   //
+  PERF_FUNCTION_END ();
   return Status;
 }
   }
@@ -148,6 +151,7 @@ SmiManage (
 // no additional handlers will be processed and EFI_INTERRUPT_PENDING 
will be returned.
 //
 if (HandlerType != NULL) {
+  PERF_FUNCTION_END ();
   return EFI_INTERRUPT_PENDING;
 }
 
@@ -160,6 +164,7 @@ SmiManage (
 // additional handlers will be processed.
 //
 if (HandlerType != NULL) {
+  PERF_FUNCTION_END ();
   return EFI_SUCCESS;
 }
 
@@ -194,6 +199,7 @@ SmiManage (
 Status = EFI_SUCCESS;
   }
 
+  PERF_FUNCTION_END ();
   return Status;
 }
 
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105495): https://edk2.groups.io/g/devel/message/105495
Mute This Topic: https://groups.io/mt/99240110/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V2 2/6] UefiCpuPkg/CpuSmm: Add perf-logging for MP procedures

[Ni, Ray]

MP procedures are those procedures that run in every CPU thread.
The EDKII perf infra is not MP safe so it doesn't support to be called
from those MP procedures.

The patch adds SMM MP perf-logging support in SmmMpPerf.c.
The following procedures are perf-logged:
* SmmInitHandler
* SmmCpuFeaturesRendezvousEntry
* PlatformValidSmi
* SmmCpuFeaturesRendezvousExit

Cc: Eric Dong 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Cc: Jiaxin Wu 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c| 34 
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 11 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.c| 91 
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.h| 77 +
 6 files changed, 216 insertions(+)
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.c
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.h

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index fa666bd118..bcd90f0671 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -778,6 +778,15 @@ BSPHandler (
   //
   WaitForAllAPs (ApCount);
 
+  //
+  // At this point, all APs should have exited from APHandler().
+  // Migrate the SMM MP performance logging to standard SMM performance 
logging.
+  // Any SMM MP performance logging after this point will be migrated in next 
SMI.
+  //
+  PERF_CODE (
+MigrateMpPerf (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
+);
+
   //
   // Reset the tokens buffer.
   //
@@ -1769,12 +1778,24 @@ SmiRendezvous (
   //
   // Perform CPU specific entry hooks
   //
+  PERF_CODE (
+MpPerfBegin (CpuIndex, SMM_MP_PERF_PROCEDURE_ID (SmmRendezvousEntry));
+);
   SmmCpuFeaturesRendezvousEntry (CpuIndex);
+  PERF_CODE (
+MpPerfEnd (CpuIndex, SMM_MP_PERF_PROCEDURE_ID (SmmRendezvousEntry));
+);
 
   //
   // Determine if this is a valid SMI
   //
+  PERF_CODE (
+MpPerfBegin (CpuIndex, SMM_MP_PERF_PROCEDURE_ID (PlatformValidSmi));
+);
   ValidSmi = PlatformValidSmi ();
+  PERF_CODE (
+MpPerfEnd (CpuIndex, SMM_MP_PERF_PROCEDURE_ID (PlatformValidSmi));
+);
 
   //
   // Determine if BSP has been already in progress. Note this must be checked 
after
@@ -1904,7 +1925,20 @@ SmiRendezvous (
   }
 
 Exit:
+  //
+  // Note: SmmRendezvousExit perf-logging entry is the only one that will be
+  //   migrated to standard perf-logging database in next SMI by 
BSPHandler().
+  //   Hence, the number of SmmRendezvousEntry entries will be larger than
+  //   the number of SmmRendezvousExit entries. Delta equals to the number
+  //   of CPU threads.
+  //
+  PERF_CODE (
+MpPerfBegin (CpuIndex, SMM_MP_PERF_PROCEDURE_ID (SmmRendezvousExit));
+);
   SmmCpuFeaturesRendezvousExit (CpuIndex);
+  PERF_CODE (
+MpPerfEnd (CpuIndex, SMM_MP_PERF_PROCEDURE_ID (SmmRendezvousExit));
+);
 
   //
   // Restore Cr2
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 2fc7dda682..5afab1e040 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -362,6 +362,9 @@ SmmInitHandler (
 
   for (Index = 0; Index < mNumberOfCpus; Index++) {
 if (ApicId == (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
+  PERF_CODE (
+MpPerfBegin (Index, SMM_MP_PERF_PROCEDURE_ID (SmmInitHandler));
+);
   //
   // Initialize SMM specific features on the currently executing CPU
   //
@@ -392,6 +395,10 @@ SmmInitHandler (
 SemaphoreHook (Index, [Index]);
   }
 
+  PERF_CODE (
+MpPerfEnd (Index, SMM_MP_PERF_PROCEDURE_ID (SmmInitHandler));
+);
+
   return;
 }
   }
@@ -699,6 +706,10 @@ PiCpuSmmEntry (
 
   gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus = mMaxNumberOfCpus;
 
+  PERF_CODE (
+InitializeMpPerf (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
+);
+
   //
   // The CPU save state and code for the SMI entry point are tiled within an 
SMRAM
   // allocated buffer.  The minimum size of this buffer for a uniprocessor 
system
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index b03f2ef882..1876a27cae 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -60,6 +60,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "CpuService.h"
 #include "SmmProfile.h"
+#include "SmmMpPerf.h"
 
 //
 // CET definition
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index af66a1941c..4864532c35 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -42,6 +42,8 @@
   SmmCpuMemoryManagement.c
   SmmMp.h
   SmmMp.c
+  SmmMpPerf.h
+  SmmMpPerf.c
 
 [Sources.Ia32]
   Ia32/Semaphore.c

[edk2-devel] [PATCH V2 1/6] UefiCpuPkg/CpuSmm: Add perf-logging for time-consuming BSP procedures

[Ni, Ray]

The patch adds perf-logging for the following potential
time-consuming BSP procedures:
* PiCpuSmmEntry
  - SmmRelocateBases
* ExecuteFirstSmiInit

* BSPHandler
  - SmmWaitForApArrival
  - PerformRemainingTasks
* InitPaging
* SetMemMapAttributes
* SetUefiMemMapAttributes
* SetPageTableAttributes
* ConfigSmmCodeAccessCheck
* SmmCpuFeaturesCompleteSmmReadyToLock

Cc: Eric Dong 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c |  8 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c| 27 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h|  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |  1 +
 .../PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c   | 13 ++---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c|  4 ++-
 6 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index baf827cf9d..fa666bd118 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -351,6 +351,8 @@ SmmWaitForApArrival (
   UINT32   DelayedCount;
   UINT32   BlockedCount;
 
+  PERF_FUNCTION_BEGIN ();
+
   DelayedCount = 0;
   BlockedCount = 0;
 
@@ -439,7 +441,7 @@ SmmWaitForApArrival (
 DEBUG ((DEBUG_INFO, "SmmWaitForApArrival: Delayed AP Count = %d, Blocked 
AP Count = %d\n", DelayedCount, BlockedCount));
   }
 
-  return;
+  PERF_FUNCTION_END ();
 }
 
 /**
@@ -577,6 +579,8 @@ BSPHandler (
   ASSERT (CpuIndex == mSmmMpSyncData->BspIndex);
   ApCount = 0;
 
+  PERF_FUNCTION_BEGIN ();
+
   //
   // Flag BSP's presence
   //
@@ -792,6 +796,8 @@ BSPHandler (
   *mSmmMpSyncData->Counter  = 0;
   *mSmmMpSyncData->AllCpusInSync= FALSE;
   mSmmMpSyncData->AllApArrivedWithException = FALSE;
+
+  PERF_FUNCTION_END ();
 }
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index c0e368ea94..2fc7dda682 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -410,12 +410,15 @@ ExecuteFirstSmiInit (
 {
   UINTN  Index;
 
+  PERF_FUNCTION_BEGIN ();
+
   if (mSmmInitialized == NULL) {
 mSmmInitialized = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * 
mMaxNumberOfCpus);
   }
 
   ASSERT (mSmmInitialized != NULL);
   if (mSmmInitialized == NULL) {
+PERF_FUNCTION_END ();
 return;
   }
 
@@ -442,6 +445,8 @@ ExecuteFirstSmiInit (
 while (!(BOOLEAN)mSmmInitialized[Index]) {
 }
   }
+
+  PERF_FUNCTION_END ();
 }
 
 /**
@@ -463,6 +468,8 @@ SmmRelocateBases (
   UINTN Index;
   UINTN BspIndex;
 
+  PERF_FUNCTION_BEGIN ();
+
   //
   // Make sure the reserved size is large enough for procedure SmmInitTemplate.
   //
@@ -540,6 +547,7 @@ SmmRelocateBases (
   //
   CopyMem (CpuStatePtr, , sizeof (BakBuf2));
   CopyMem (U8Ptr, BakBuf, sizeof (BakBuf));
+  PERF_FUNCTION_END ();
 }
 
 /**
@@ -617,6 +625,8 @@ PiCpuSmmEntry (
   GuidHob= NULL;
   SmmBaseHobData = NULL;
 
+  PERF_FUNCTION_BEGIN ();
+
   //
   // Initialize address fixup
   //
@@ -1194,6 +1204,7 @@ PiCpuSmmEntry (
 
   DEBUG ((DEBUG_INFO, "SMM CPU Module exit from SMRAM with EFI_SUCCESS\n"));
 
+  PERF_FUNCTION_END ();
   return EFI_SUCCESS;
 }
 
@@ -1348,12 +1359,15 @@ ConfigSmmCodeAccessCheck (
   UINTN   Index;
   EFI_STATUS  Status;
 
+  PERF_FUNCTION_BEGIN ();
+
   //
   // Check to see if the Feature Control MSR is supported on this CPU
   //
   Index = gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu;
   if (!SmmCpuFeaturesIsSmmRegisterSupported (Index, SmmRegFeatureControl)) {
 mSmmCodeAccessCheckEnable = FALSE;
+PERF_FUNCTION_END ();
 return;
   }
 
@@ -1363,6 +1377,7 @@ ConfigSmmCodeAccessCheck (
   //
   if ((AsmReadMsr64 (EFI_MSR_SMM_MCA_CAP) & SMM_CODE_ACCESS_CHK_BIT) == 0) {
 mSmmCodeAccessCheckEnable = FALSE;
+PERF_FUNCTION_END ();
 return;
   }
 
@@ -1419,6 +1434,8 @@ ConfigSmmCodeAccessCheck (
   ReleaseSpinLock (mConfigSmmCodeAccessCheckLock);
 }
   }
+
+  PERF_FUNCTION_END ();
 }
 
 /**
@@ -1540,6 +1557,8 @@ PerformRemainingTasks (
   )
 {
   if (mSmmReadyToLock) {
+PERF_FUNCTION_BEGIN ();
+
 //
 // Start SMM Profile feature
 //
@@ -1574,12 +1593,20 @@ PerformRemainingTasks (
 //
 ConfigSmmCodeAccessCheck ();
 
+//
+// Measure performance of SmmCpuFeaturesCompleteSmmReadyToLock() from 
caller side
+// as the implementation is provided by platform.
+//
+PERF_START (NULL, "SmmCompleteReadyToLock", NULL, 0);
 SmmCpuFeaturesCompleteSmmReadyToLock ();
+PERF_END (NULL, "SmmCompleteReadyToLock", NULL, 0);
 
 //
 // Clean SMM ready to lock flag
 //
 mSmmReadyToLock = FALSE;
+
+PERF_FUNCTION_END ();
   }
 }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index a5c2bdd971..b03f2ef882 100644
--- 

[edk2-devel] [PATCH V2 0/6] Enable perf-logging in SMM environment

[Ni, Ray]

Ray Ni (6):
  UefiCpuPkg/CpuSmm: Add perf-logging for time-consuming BSP procedures
  UefiCpuPkg/CpuSmm: Add perf-logging for MP procedures
  MdeModulePkg/SmmCore: Add perf-logging for time-consuming procedures
  MdeModulePkg/SmmCore: Add perf-logging for SmmDriverDispatchHandler
  MdeModulePkg/SmmPerformanceLib: Disable perf-logging after ExitBS
  MdeModulePkg/SmmCorePerformanceLib: Disable perf-logging at runtime

 MdeModulePkg/Core/PiSmmCore/Dispatcher.c  |  5 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c   | 14 ++-
 MdeModulePkg/Core/PiSmmCore/Smi.c |  6 ++
 .../SmmCorePerformanceLib.c   | 48 +-
 .../SmmCorePerformanceLib.inf |  3 +-
 .../SmmPerformanceLib/SmmPerformanceLib.c | 63 -
 .../SmmPerformanceLib/SmmPerformanceLib.inf   |  4 +
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 42 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c| 38 
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h|  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |  3 +
 .../PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c   | 13 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.c | 91 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.h | 77 
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c|  4 +-
 15 files changed, 402 insertions(+), 11 deletions(-)
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.c
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMpPerf.h

-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105492): https://edk2.groups.io/g/devel/message/105492
Mute This Topic: https://groups.io/mt/99240107/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] RDTSC is not a serializing instruction

[Wu, Jiaxin]

It does has the impact, I have observed the phenomenon due to speculation 
execution.

Thanks,
Jiaxin

> -Original Message-
> From: Ni, Ray 
> Sent: Thursday, May 25, 2023 2:11 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray ; Wu, Jiaxin ; Wu, Hao A
> 
> Subject: RDTSC is not a serializing instruction
> 
> All,
> According to below Intel SDM content:
> > The RDTSC instruction is not a serializing instruction. It does not 
> > necessarily
> wait until all previous instructions have been executed before reading the
> counter.
> > Similarly, subsequent instructions may begin execution before the read
> operation is performed. The following items may guide software seeking to
> order executions of RDTSC:
> > If software requires RDTSC to be executed only after all previous
> instructions have executed and all previous
> >   loads are globally visible,1 it can execute LFENCE immediately before
> RDTSC.
> > If software requires RDTSC to be executed only after all previous
> instructions have executed and all previous
> >   loads and stores are globally visible, it can execute the sequence
> MFENCE;LFENCE immediately before RDTSC.
> > If software requires RDTSC to be executed prior to execution of any
> subsequent instruction (including any
> >   memory accesses), it can execute the sequence LFENCE immediately
> after RDTSC.
> 
> RDTSC might not return the accurate "current" tick.
> A robust implementation could be "lfence" then "rdtsc". Otherwise, the edk2
> perf-logging infra might generate incorrect data if using
> UefiCpuPkg/CpuTimerLib.
> 
> Lots of code polling for registers within a certain timeout is also impacted. 
> I
> am curious why "timeout" doesn't happen. Or maybe "timeout" did happen
> that resulted in a longer "timeout" value was used?
> 
> Comments?
> 
> Thanks,
> Ray


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105491): https://edk2.groups.io/g/devel/message/105491
Mute This Topic: https://groups.io/mt/99124748/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI

[Ard Biesheuvel]

On Wed, 31 May 2023 at 10:56, Ni, Ray  wrote:
>
> > > > > 2.
> > > > > When a memory region is marked from not-present to present,
> > PageTableLib
> > > > expects
> > > > > caller to supply all memory attributes (including RW, NX, etc.) as 
> > > > > the lib
> > > > implementation doesn't
> > > > > want to carry any default attributes..
> > > > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > > > >
> > > >
> > > > I'm not sure I follow.
> > > >
> > > > The PPI (as well as the UEFI protocol) can only operate on valid
> > > > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > > > used to create mappings from scratch.
> > > When a range of memory is marked as "RP", X86 page table clears the
> > > "Present" bit for that range memory.
> > > "Present" bit is a master bit in X86 page table. When that bit is clear, 
> > > all
> > > other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
> > >
> > > So, if caller clears the "RP" bit (setting "Present" bit in page table), 
> > > that's an
> > > operation to map back some memory.
> > > X86 CpuPageTableLib requires all attributes be provided for mapping back
> > > some memory.
> > >
> > > >
> > > > Do you think this capability should be added? If so, I think it is
> > > > reasonable to require the caller to provide all attributes, and on ARM
> > > > this would have to include the memory cacheability type as we should
> > > > not provide a default for that either.
> > >
> > > Yes. I think this is required. Having this rule can help caller write 
> > > robust code
> > > instead of depending on some default attributes in PPI/Protocol
> > implementation.
> > >
> >
> > I still don't follow. How does that work in the context of the
> > attribute mask? Can you given some examples?
>
> OK. Let's reset the discussion.
> For example, one caller's code as below:
>   // mark 0-4k as not-present
>   MemoryAttrributePpi->SetMemoryAttribute (0, 4K, RP, RP); // Use 
> Attribute/Mask pattern to set RP
>
> Another caller's code:
> // mark 0-4k as present
> * MemoryAttributePpi->SetMemoryAttribute (0, 4K, 0, RP); // Use 
> Attribute/Mask pattern to clear RP
>

OK, I see what you mean now.

> Q1: Does the PPI support this usage?

My current implementation for ARM does not support this, because the
RP flag is not tied to the present bit but to the access flag. This
means that setting and clearing RP will not create a valid region.

> Q2: If it supports, what're the other attributes of 0-4k memory? Is XP set? 
> Is RO set?
>

I am leaning towards not supporting this, at least initially, as
updating permissions and creating mappings are two different things,
and I am not convinced a generic PPI for creating mappings from
scratch is needed at this point.

This is a major difference between ARM and x86, as on x86, it is quite
common to create mappings for regions that may have no memory at all.
On ARM, this is not permited, and so we carefully create mappings only
where we detect DRAM.

However, I understand that it is not trivial to implement this
restriction on x86, unless we remove RP from the set of attributes
that this API can manipulate.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105490): https://edk2.groups.io/g/devel/message/105490
Mute This Topic: https://groups.io/mt/99131184/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] MinPlatform: Add CpuPageTableLib required by CpuMpPeim

[Ni, Ray]

The patch moves the CpuPageTableLib reference from CoreDxeLib.dsc
to CoreCommonLib.dsc since now not only DxeMpInitLib but also CpuMpPei
depends on it.

Signed-off-by: Ray Ni 
Cc: Chasel Chiu 
Cc: Nate DeSimone 
Cc: Isaac Oram 
Cc: Liming Gao 
Cc: Eric Dong 
---
 Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc | 1 +
 Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc| 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc 
b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
index dfe7d836d3..300b7d7652 100644
--- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
+++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
@@ -106,6 +106,7 @@
   LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
   SmmCpuFeaturesLib|UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
   MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+  CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
 
   #
   # Platform
diff --git a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc 
b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
index af30f985ca..5a2cb130b3 100644
--- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
+++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc
@@ -1,7 +1,7 @@
 ## @file
 #  Platform description.
 #
-# Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2023, Intel Corporation. All rights reserved.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -27,7 +27,6 @@
   LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf
 
   
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
-  CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
   TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105489): https://edk2.groups.io/g/devel/message/105489
Mute This Topic: https://groups.io/mt/99238376/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] [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI

[Ni, Ray]

> > > > 2.
> > > > When a memory region is marked from not-present to present,
> PageTableLib
> > > expects
> > > > caller to supply all memory attributes (including RW, NX, etc.) as the 
> > > > lib
> > > implementation doesn't
> > > > want to carry any default attributes..
> > > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > > >
> > >
> > > I'm not sure I follow.
> > >
> > > The PPI (as well as the UEFI protocol) can only operate on valid
> > > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > > used to create mappings from scratch.
> > When a range of memory is marked as "RP", X86 page table clears the
> > "Present" bit for that range memory.
> > "Present" bit is a master bit in X86 page table. When that bit is clear, all
> > other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
> >
> > So, if caller clears the "RP" bit (setting "Present" bit in page table), 
> > that's an
> > operation to map back some memory.
> > X86 CpuPageTableLib requires all attributes be provided for mapping back
> > some memory.
> >
> > >
> > > Do you think this capability should be added? If so, I think it is
> > > reasonable to require the caller to provide all attributes, and on ARM
> > > this would have to include the memory cacheability type as we should
> > > not provide a default for that either.
> >
> > Yes. I think this is required. Having this rule can help caller write 
> > robust code
> > instead of depending on some default attributes in PPI/Protocol
> implementation.
> >
> 
> I still don't follow. How does that work in the context of the
> attribute mask? Can you given some examples?

OK. Let's reset the discussion.
For example, one caller's code as below:
  // mark 0-4k as not-present
  MemoryAttrributePpi->SetMemoryAttribute (0, 4K, RP, RP); // Use 
Attribute/Mask pattern to set RP

Another caller's code:
// mark 0-4k as present
* MemoryAttributePpi->SetMemoryAttribute (0, 4K, 0, RP); // Use Attribute/Mask 
pattern to clear RP

Q1: Does the PPI support this usage?
Q2: If it supports, what're the other attributes of 0-4k memory? Is XP set? Is 
RO set?




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105488): https://edk2.groups.io/g/devel/message/105488
Mute This Topic: https://groups.io/mt/99131184/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] [edk2-platforms][PATCH 2/2] ManageabilityPkg: Use DEBUG_MANAGEABILITY

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Merged.

Thanks
Abner

> -Original Message-
> From: Attar, AbdulLateef (Abdul Lateef) 
> Sent: Tuesday, May 30, 2023 4:48 PM
> To: Chang, Abner ; devel@edk2.groups.io
> Cc: Isaac Oram ; Nickle Wang
> ; Tinh Nguyen
> 
> Subject: RE: [edk2-platforms][PATCH 2/2] ManageabilityPkg: Use
> DEBUG_MANAGEABILITY
>
> [AMD Official Use Only - General]
>
> Reviewed-by: Abdul Lateef Attar 
>
> -Original Message-
> From: Chang, Abner 
> Sent: Tuesday, May 30, 2023 12:03 PM
> To: devel@edk2.groups.io
> Cc: Isaac Oram ; Attar, AbdulLateef (Abdul Lateef)
> ; Nickle Wang ; Tinh
> Nguyen 
> Subject: [edk2-platforms][PATCH 2/2] ManageabilityPkg: Use
> DEBUG_MANAGEABILITY
>
> From: Abner Chang 
>
> Use debug print level DEBUG_MANAGEABILITY in ManageabilityPkg.
>
> Signed-off-by: Abner Chang 
> Cc: Isaac Oram 
> Cc: Abdul Lateef Attar 
> Cc: Nickle Wang 
> Cc: Tinh Nguyen 
> ---
>  .../Library/ManageabilityTransportHelperLib.h  |  2 +-
>  .../BaseManageabilityTransportHelper.c |  2 +-
>  .../Universal/IpmiBmcAcpi/BmcAcpi.c|  6 --
>  .../Universal/IpmiBmcElog/BmcElog.c|  4 +++-
>  .../ManageabilityPkg/Universal/IpmiFrb/FrbDxe.c|  8 +---
>  .../PldmSmbiosTransferDxe/PldmSmbiosTransferDxe.c  | 14 +++---
>  6 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git
> a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelperL
> ib.h
> b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelper
> Lib.h
> index 11a1bd0521..dfe32189ad 100644
> ---
> a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelperL
> ib.h
> +++ b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHe
> +++ lperLib.h
> @@ -11,7 +11,7 @@
>
>  #include 
>
> -#define DEBUG_MANAGEABILITY_INFO  DEBUG_INFO
> +#define DEBUG_MANAGEABILITY_INFO  DEBUG_MANAGEABILITY
>
>  typedef struct _MANAGEABILITY_PROTOCOL_NAME
> MANAGEABILITY_PROTOCOL_NAME;
>
> diff --git
> a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/
> BaseManageabilityTransportHelper.c
> b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/
> BaseManageabilityTransportHelper.c
> index f72957ea7f..27bc5eaddf 100644
> ---
> a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/
> BaseManageabilityTransportHelper.c
> +++ b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelper
> +++ Lib/BaseManageabilityTransportHelper.c
> @@ -163,7 +163,7 @@ HelperAcquireManageabilityTransport (
>CHAR16  *ManageabilityProtocolName;
>CHAR16  *ManageabilityTransportName;
>
> -  DEBUG ((DEBUG_INFO, "%a: Entry\n", __func__));
> +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Entry\n", __func__));
>if ((TransportToken == NULL) || (ManageabilityProtocolSpec == NULL)) {
>  DEBUG ((DEBUG_ERROR, "%a: One of the required input parameters is
> NULL.\n", __func__));
>  return EFI_INVALID_PARAMETER;
> diff --git a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> index cf066dd095..d04623ecad 100644
> --- a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> +++ b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> @@ -24,6 +24,8 @@
>  #include 
>  #include 
>
> +#include 
> +
>  #ifndef EFI_ACPI_CREATOR_ID
>  #define EFI_ACPI_CREATOR_ID  SIGNATURE_32 ('M', 'S', 'F', 'T')  #endif @@ -
> 140,7 +142,7 @@ UpdateDeviceSsdtTable (
>//
>// Update IO(Decode16, 0xCA2, 0xCA2, 0, 2)
>//
> -  DEBUG ((DEBUG_INFO, "UpdateDeviceSsdtTable - IPMI\n"));
> +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "UpdateDeviceSsdtTable -
> IPMI\n"));
>for (DataPtr = (UINT8 *)(Table + 1);
> DataPtr < (UINT8 *)((UINT8 *)Table + Table->Length - 4);
> DataPtr++)
> @@ -158,7 +160,7 @@ UpdateDeviceSsdtTable (
>ASSERT (IoRsc->Header.Bits.Type == ACPI_SMALL_ITEM_FLAG);
>ASSERT (IoRsc->Header.Bits.Name ==
> ACPI_SMALL_IO_PORT_DESCRIPTOR_NAME);
>ASSERT (IoRsc->Header.Bits.Length == sizeof
> (EFI_ACPI_IO_PORT_DESCRIPTOR) - sizeof
> (ACPI_SMALL_RESOURCE_HEADER));
> -  DEBUG ((DEBUG_INFO, "IPMI IO Base in ASL update - 0x%04x <=
> 0x%04x\n", IoRsc->BaseAddressMin, PcdGet16 (PcdIpmiKcsIoBaseAddress)));
> +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "IPMI IO Base in ASL update -
> + 0x%04x <= 0x%04x\n", IoRsc->BaseAddressMin, PcdGet16
> + (PcdIpmiKcsIoBaseAddress)));
>IoRsc->BaseAddressMin = PcdGet16 (PcdIpmiKcsIoBaseAddress);
>IoRsc->BaseAddressMax = PcdGet16 (PcdIpmiKcsIoBaseAddress);
>  }
> diff --git a/Features/ManageabilityPkg/Universal/IpmiBmcElog/BmcElog.c
> b/Features/ManageabilityPkg/Universal/IpmiBmcElog/BmcElog.c
> index 02873fc4c6..8b34b2d2d5 100644
> --- a/Features/ManageabilityPkg/Universal/IpmiBmcElog/BmcElog.c
> +++ b/Features/ManageabilityPkg/Universal/IpmiBmcElog/BmcElog.c
> @@ -15,6 +15,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  #include
> 

Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers

[Marvin Häuser]

Hi Ray,This is handled by GenFw. For ELF-based toolchains, this is done as part of the conversion [1]. For others, a “GenFw post-processing” step takes care of it [2]. Though you are right, this is not obvious and external tools may forget this step (but still produce valid XIP-alike PEs). Consequently, I guess I’ll recommend against my former suggestion to check FileAlign == SectAlign and to instead just check the actual section info [3].While this looks safe for now, GenFv would be a much more logical place for XIP conversion. This way, everything is XIP and when page-aligning all images, the flash storage quickly runs out of space (at least without compression). That GenFv has no logic to grow image files also kept me from implementing a new 2-section approach to XIP where metadata and sections are stored separately, such that the header metadata doesn’t need to be section-aligned.(Completely irrelevant to this discussion, but while we’re at it, the flat storage hierarchy also makes memory protection for XIP a pain, as there will be FFS metadata inbetween 4K-aligned (or maybe even more in the future?) images, thus virtually always yielding padding. If all images were stored consecutively and the FFS metadata rather pointed to their locations, a lot if padding could be avoided. Though this would need some analysis as to how much you actually save, as adding an offset variable will obviously grow the FFS overhead. It would also be nice to somehow encode the offset implicitly, where the first one is explicitly provided and the rest is just adding up the size of the previous one, but that might screw over some existing parsers.)Best regards,Marvin[1] https://github.com/tianocore/edk2/blob/d8e5d35ede7158ccbb9abf600e65b9aa6e043f74/BaseTools/Source/C/GenFw/Elf64Convert.c#L1297[2] https://github.com/tianocore/edk2/blob/d8e5d35ede7158ccbb9abf600e65b9aa6e043f74/BaseTools/Source/C/GenFw/GenFw.c#L2088[3] https://github.com/acidanthera/audk/blob/d9bb10ae3b73134eb434b309cb0db3fa4282a838/MdePkg/Library/BasePeCoffLib2/PeCoffLoad.c#L134On 31. May 2023, at 09:14, Ni, Ray  wrote:







When a PE has a global large variable, there could be a .bss section whose actual size is 0 or very small but the eventual size when loading to memory will be larger.
Can XIP work with this section? 
@Liu, Zhiguang brought this question when discussing with me offline
 
Thanks,
Ray
 


From: devel@edk2.groups.io  
On Behalf Of Marvin Häuser
Sent: Tuesday, May 30, 2023 6:25 PM
To: Ard Biesheuvel 
Cc: edk2-devel-groups-io ; Ni, Ray 
Subject: Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers


 
 



On 30. May 2023, at 12:02, Ard Biesheuvel  wrote:

 

On Tue, 30 May 2023 at 11:53, Marvin Häuser 
 wrote:






On 30. May 2023, at 11:48, Ard Biesheuvel  wrote:

On Tue, 30 May 2023 at 11:47, Ard Biesheuvel  wrote:


On Tue, 30 May 2023 at 11:42, Marvin Häuser  wrote:


I took a *very brief* look at the entire series now. Is this just to apply permissions before CpuDxe is loaded


Yes.


Well, actually, the first part of the series gets rid of some
pointless memory copies, which is an improvement in itself.


Sorry, I didn't mean the series, I meant the choice to handle things in DxeIpl over DxeCore in particular.

Is there even a non-FOSS producer of the CpuArch protocol? To my understanding, all (common) Intel platforms use the one in UefiCpuPkg. ARM and friends feel a lot less proprietary to begin with, but I could be wrong. We were quite successful so far with just
 merging CpuArch into DxeCore and setting it up quite early, but of course not at an industry-wide scale. :)


What about the dependencies of CpuArch protocol? On ARM, this is the
GIC driver (for the interrupt controller), which has its own platform
specific dependencies.



 


Hmm, was that for the exception handler? I forgot that was in CpuDxe too, I specifically meant the memory permission related things, sorry!







So this is not tractable in general, and the only options we have (imo) are

- add memory permission attribute handling to DxeCore directly (via a
library with arch specific implementations)



 


Yes, this.






- add a way to dispatch the CpuDxe *and its dependencies* without the
need to manipulate memory permissions



 


That would be awful and I'd prefer your current solution over this.







Clumping everything together into DxeCore does not appear to me as a
sustainable approach for this.



 









_._,_._,_



Groups.io Links:


  
You receive all messages sent to this group.
  
  



View/Reply Online (#105486) |


  

|

  Mute This Topic


| New Topic






Your Subscription |
Contact Group Owner |

Unsubscribe

 [arch...@mail-archive.com]
_._,_._,_

Re: [edk2-devel] [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI

[Ard Biesheuvel]

On Wed, 31 May 2023 at 09:34, Ni, Ray  wrote:
>
>
>
> > -Original Message-
> > From: Ard Biesheuvel 
> > Sent: Tuesday, May 30, 2023 3:32 PM
> > To: Ni, Ray 
> > Cc: devel@edk2.groups.io; Yao, Jiewen ; Gerd
> > Hoffmann ; Taylor Beebe ; Oliver
> > Smith-Denny ; Bi, Dandan ;
> > Gao, Liming ; Kinney, Michael D
> > ; Leif Lindholm ;
> > Sunil V L ; Warkentin, Andrei
> > 
> > Subject: Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> >
> > On Tue, 30 May 2023 at 09:15, Ni, Ray  wrote:
> > >
> > > 1.
> > > The PPI interface supports to set and clear any attributes with single
> > invocation.
> > > That's much better than the existing UEFI protocol prototype which 
> > > requires
> > caller to call the interfaces
> > > twice to set and clear some attributes.
> > >
> >
> > Agree, I think that was a mistake to define the UEFI memory attributes
> > protocol like that, as it requires two traversals of the page tables
> > for the most common case of converting RO -> XP or vice versa.
> >
> > > So far I see two patterns for attributes setting:
> > > *. The patten in this patch: SetMask/ClearMask
> > > *. The pattern I used in PageTableLib: Attribute/Mask.
> > >
> > > I think from caller side, they provide the same capabilities.
> > > The difference is SetMask/ClearMask expects callers to not set the same 
> > > BIT
> > in both
> > > SetMask/ClearMask.
> > >
> > > (I thought there would be similar existing interfaces as pattern 2. But I 
> > > didn't
> > find any now.)
> > > Do you mind to align to pattern #2?
> > >
> >
> > That is fine - I actually prefer that (and this is what ArmMmuLib
> > implements internally) but I did not want to deviate from the UEFI
> > protocol too much.
>
> By adding "ClearMask", you already made something different
> Good to know you prefer pattern #2.
>

Yeah :-)


> >
> > >
> > > 2.
> > > When a memory region is marked from not-present to present, PageTableLib
> > expects
> > > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> > implementation doesn't
> > > want to carry any default attributes..
> > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > >
> >
> > I'm not sure I follow.
> >
> > The PPI (as well as the UEFI protocol) can only operate on valid
> > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > used to create mappings from scratch.
> When a range of memory is marked as "RP", X86 page table clears the
> "Present" bit for that range memory.
> "Present" bit is a master bit in X86 page table. When that bit is clear, all
> other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
>
> So, if caller clears the "RP" bit (setting "Present" bit in page table), 
> that's an
> operation to map back some memory.
> X86 CpuPageTableLib requires all attributes be provided for mapping back
> some memory.
>
> >
> > Do you think this capability should be added? If so, I think it is
> > reasonable to require the caller to provide all attributes, and on ARM
> > this would have to include the memory cacheability type as we should
> > not provide a default for that either.
>
> Yes. I think this is required. Having this rule can help caller write robust 
> code
> instead of depending on some default attributes in PPI/Protocol 
> implementation.
>

I still don't follow. How does that work in the context of the
attribute mask? Can you given some examples?

Creating new memory mappings from scratch is a totally different use
case, so perhaps this should be a separate PPI method.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105485): https://edk2.groups.io/g/devel/message/105485
Mute This Topic: https://groups.io/mt/99131184/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] OvmfPkg/VirtIoSerialDxe: Update for VS2015x86 compatibility

[Gerd Hoffmann]

On Fri, May 26, 2023 at 04:17:34PM -0700, Michael D Kinney wrote:
> Move initialization of local variable structure from declaration
> to statements to fix VS2015x86 build break.

Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105483): https://edk2.groups.io/g/devel/message/105483
Mute This Topic: https://groups.io/mt/99160001/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI

[Ni, Ray]

> -Original Message-
> From: Ard Biesheuvel 
> Sent: Tuesday, May 30, 2023 3:32 PM
> To: Ni, Ray 
> Cc: devel@edk2.groups.io; Yao, Jiewen ; Gerd
> Hoffmann ; Taylor Beebe ; Oliver
> Smith-Denny ; Bi, Dandan ;
> Gao, Liming ; Kinney, Michael D
> ; Leif Lindholm ;
> Sunil V L ; Warkentin, Andrei
> 
> Subject: Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> 
> On Tue, 30 May 2023 at 09:15, Ni, Ray  wrote:
> >
> > 1.
> > The PPI interface supports to set and clear any attributes with single
> invocation.
> > That's much better than the existing UEFI protocol prototype which requires
> caller to call the interfaces
> > twice to set and clear some attributes.
> >
> 
> Agree, I think that was a mistake to define the UEFI memory attributes
> protocol like that, as it requires two traversals of the page tables
> for the most common case of converting RO -> XP or vice versa.
> 
> > So far I see two patterns for attributes setting:
> > *. The patten in this patch: SetMask/ClearMask
> > *. The pattern I used in PageTableLib: Attribute/Mask.
> >
> > I think from caller side, they provide the same capabilities.
> > The difference is SetMask/ClearMask expects callers to not set the same BIT
> in both
> > SetMask/ClearMask.
> >
> > (I thought there would be similar existing interfaces as pattern 2. But I 
> > didn't
> find any now.)
> > Do you mind to align to pattern #2?
> >
> 
> That is fine - I actually prefer that (and this is what ArmMmuLib
> implements internally) but I did not want to deviate from the UEFI
> protocol too much.

By adding "ClearMask", you already made something different
Good to know you prefer pattern #2.

> 
> >
> > 2.
> > When a memory region is marked from not-present to present, PageTableLib
> expects
> > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> implementation doesn't
> > want to carry any default attributes..
> > Do you think the MemoryAttribute PPI should expect the same to caller?
> >
> 
> I'm not sure I follow.
> 
> The PPI (as well as the UEFI protocol) can only operate on valid
> mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> used to create mappings from scratch.
When a range of memory is marked as "RP", X86 page table clears the 
"Present" bit for that range memory.
"Present" bit is a master bit in X86 page table. When that bit is clear, all
other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.

So, if caller clears the "RP" bit (setting "Present" bit in page table), that's 
an
operation to map back some memory.
X86 CpuPageTableLib requires all attributes be provided for mapping back
some memory.

> 
> Do you think this capability should be added? If so, I think it is
> reasonable to require the caller to provide all attributes, and on ARM
> this would have to include the memory cacheability type as we should
> not provide a default for that either.

Yes. I think this is required. Having this rule can help caller write robust 
code
instead of depending on some default attributes in PPI/Protocol implementation.

> 
> Thanks,
> Ard.
> 
> 
> >
> >
> > > -Original Message-
> > > From: Ard Biesheuvel 
> > > Sent: Thursday, May 25, 2023 10:31 PM
> > > To: devel@edk2.groups.io
> > > Cc: Ard Biesheuvel ; Ni, Ray ; Yao,
> Jiewen
> > > ; Gerd Hoffmann ; Taylor
> Beebe
> > > ; Oliver Smith-Denny ; Bi,
> Dandan
> > > ; Gao, Liming ;
> Kinney,
> > > Michael D ; Leif Lindholm
> > > ; Sunil V L ;
> Warkentin,
> > > Andrei 
> > > Subject: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> > >
> > > Define a PPI interface that may be used by the PEI core or other PEIMs
> > > to manage permissions on memory ranges. This is primarily intended for
> > > restricting permissions to what is actually needed for correct execution
> > > by the code in question, and for limiting the use of memory mappings
> > > that are both writable and executable at the same time.
> > >
> > > Signed-off-by: Ard Biesheuvel 
> > > ---
> > >  MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78
> 
> > >  MdeModulePkg/MdeModulePkg.dec  |  3 +
> > >  2 files changed, 81 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > > b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > > new file mode 100644
> > > index ..5ff31185ab4183f8
> > > --- /dev/null
> > > +++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > > @@ -0,0 +1,78 @@
> > > +/** @file
> > >
> > > +
> > >
> > > +Copyright (c) 2023, Google LLC. All rights reserved.
> > >
> > > +
> > >
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > +
> > >
> > > +**/
> > >
> > > +
> > >
> > > +#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_
> > >
> > > +#define EDKII_MEMORY_ATTRIBUTE_PPI_H_
> > >
> > > +
> > >
> > > +#include 
> > >
> > > +
> > >
> > > +///
> > >
> > > +/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.
> > >
> > > +///
> > >
> > > +#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \
> > >
> > > 

Re: [edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX

[Gerd Hoffmann]

  Hi,

> However, I must admit that the X64 PEI logic is confusing to me, so I
> may have missed something here. It seems to me that creating an
> entirely new set of page tables in DxeIpl is both redundant (as PEI
> already executes in long mode, and therefore uses page tables)

Well, there is the 32bit PEI / 64bit DXE case.  Which might be the
reason why a new set of page tables is created (unconditionally) even
though it should not be needed in the 64bit PEI / 64bit DXE case.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105482): https://edk2.groups.io/g/devel/message/105482
Mute This Topic: https://groups.io/mt/99131196/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and remap XIP capable DXE drivers

[Ni, Ray]

When a PE has a global large variable, there could be a .bss section whose 
actual size is 0 or very small but the eventual size when loading to memory 
will be larger.
Can XIP work with this section? @Liu, Zhiguang 
brought this question when discussing with me offline

Thanks,
Ray

From: devel@edk2.groups.io  On Behalf Of Marvin Häuser
Sent: Tuesday, May 30, 2023 6:25 PM
To: Ard Biesheuvel 
Cc: edk2-devel-groups-io ; Ni, Ray 
Subject: Re: [edk2-devel] [RFC PATCH 08/11] MdeModulePkg/DxeIpl: Relocate and 
remap XIP capable DXE drivers


On 30. May 2023, at 12:02, Ard Biesheuvel 
mailto:a...@kernel.org>> wrote:

On Tue, 30 May 2023 at 11:53, Marvin Häuser 
mailto:mhaeu...@posteo.de>> wrote:




On 30. May 2023, at 11:48, Ard Biesheuvel 
mailto:a...@kernel.org>> wrote:

On Tue, 30 May 2023 at 11:47, Ard Biesheuvel 
mailto:a...@kernel.org>> wrote:


On Tue, 30 May 2023 at 11:42, Marvin Häuser 
mailto:mhaeu...@posteo.de>> wrote:


I took a *very brief* look at the entire series now. Is this just to apply 
permissions before CpuDxe is loaded


Yes.


Well, actually, the first part of the series gets rid of some
pointless memory copies, which is an improvement in itself.


Sorry, I didn't mean the series, I meant the choice to handle things in DxeIpl 
over DxeCore in particular.

Is there even a non-FOSS producer of the CpuArch protocol? To my understanding, 
all (common) Intel platforms use the one in UefiCpuPkg. ARM and friends feel a 
lot less proprietary to begin with, but I could be wrong. We were quite 
successful so far with just merging CpuArch into DxeCore and setting it up 
quite early, but of course not at an industry-wide scale. :)

What about the dependencies of CpuArch protocol? On ARM, this is the
GIC driver (for the interrupt controller), which has its own platform
specific dependencies.

Hmm, was that for the exception handler? I forgot that was in CpuDxe too, I 
specifically meant the memory permission related things, sorry!



So this is not tractable in general, and the only options we have (imo) are

- add memory permission attribute handling to DxeCore directly (via a
library with arch specific implementations)

Yes, this.


- add a way to dispatch the CpuDxe *and its dependencies* without the
need to manipulate memory permissions

That would be awful and I'd prefer your current solution over this.



Clumping everything together into DxeCore does not appear to me as a
sustainable approach for this.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105481): https://edk2.groups.io/g/devel/message/105481
Mute This Topic: https://groups.io/mt/99197142/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 1/1] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection

[Gerd Hoffmann]

On Thu, May 25, 2023 at 05:53:27PM +0200, Ard Biesheuvel wrote:
> On Tue, 2 May 2023 at 07:59, Gerd Hoffmann  wrote:
> >
> > Check whenever flash is actually writable.
> >
> 
> This is a bit too terse. Could you explain why this is needed, and why
> this approach is suitable?

Flash can be write-protected in qemu (which is usually the case for
code).  In case the variable store flash block is configured read-only
ovmf wouldn't be able to store EFI variables there, so not setting up
fvb in that case (and fallhack to emulation) is the better option.
It'll avoid problems later due to flash writes failing.

The patch tries to write back the original value read earlier, so flash
content doesn't change in case the write succeeds.  But the status we
read back after the attempt to write will tell us whenever flash is
writable or not.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105480): https://edk2.groups.io/g/devel/message/105480
Mute This Topic: https://groups.io/mt/98633328/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v10] MinPlatformPkg: Update HWSignature field in FACS

[VincentX Ke]

Thanks for review and merge.

-Original Message-
From: Chiu, Chasel  
Sent: Wednesday, May 31, 2023 1:38 PM
To: Ke, VincentX ; devel@edk2.groups.io
Cc: Desimone, Nathaniel L ; Oram, Isaac W 
; Gao, Liming ; Dong, Eric 
; Sinha, Ankit ; Ni, Ray 

Subject: RE: [PATCH v10] MinPlatformPkg: Update HWSignature field in FACS


Patch merged:
https://github.com/tianocore/edk2-platforms/commit/1a7bd150d39007bfb72c4727feda3184c23efe96

Thanks,
Chasel


> -Original Message-
> From: Ke, VincentX 
> Sent: Tuesday, May 30, 2023 7:48 PM
> To: devel@edk2.groups.io
> Cc: Ke, VincentX ; Chiu, Chasel 
> ; Desimone, Nathaniel L 
> ; Oram, Isaac W 
> ; Gao, Liming ; 
> Dong, Eric ; Sinha, Ankit 
> Subject: [PATCH v10] MinPlatformPkg: Update HWSignature field in FACS
> 
> From: VincentX Ke 
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
> 
> Calculating CRC based on each ACPI table.
> Update HWSignature field 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   | 284
> +++-
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
>  2 files changed, 217 insertions(+), 68 deletions(-)
> 
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index 2f2c96f907..2a833ec99c 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,246 @@ 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.
> 
> +  Function prototype for GetAcpiTableCount/CalculateAcpiTableCrc.
> 
> +
> 
> +  @param[in] TableThe pointer to ACPI table.
> 
> +  @param[in] TableIndex   The ACPI table index.
> 
> +  @param[in] Context  The pointer to UINTN for GetAcpiTableCount.
> 
> +  The pointer to UINT32 array for 
> CalculateAcpiTableCrc.
> 
>  **/
> 
> +typedef
> 
>  VOID
> 
> -IsHardwareChange (
> 
> -  VOID
> 
> +(EFIAPI *ACPI_TABLE_CALLBACK)(
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> 
> +  IN  UINTN   TableIndex,
> 
> +  IN  VOID*Context
> 
> +  );
> 
> +
> 
> +/**
> 
> +  Enumerate all ACPI tables in RSDT/XSDT.
> 
> +
> 
> +  @param[in] SdtACPI XSDT/RSDT.
> 
> +  @param[in] TablePointerSize   Size of table pointer:
> 
> +4(RSDT) or 8(XSDT).
> 
> +  @param[in] CallbackFunction   The pointer to
> GetAcpiTableCount/CalculateAcpiTableCrc.
> 
> +  @param[in] ContextThe pointer to UINTN for GetAcpiTableCount.
> 
> +The pointer to UINT32 array for 
> CalculateAcpiTableCrc.
> 
> +**/
> 
> +VOID
> 
> +EnumerateAllAcpiTables (
> 
> +  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
> 
> +  IN  UINTNTablePointerSize,
> 
> +  IN  ACPI_TABLE_CALLBACK  CallbackFunction,
> 
> +  IN  VOID *Context
> 
>)
> 
>  {
> 
> -  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!
> 
> +  UINTN  Index;
> 
> +  UINTN  TableIndex;
> 
> +  UINTN  EntryCount;
> 
> +  UINT64 EntryPtr;
> 
> +  UINTN  BasePtr;
> 
> +  EFI_ACPI_COMMON_HEADER *Table;
>