Date   

回复: [edk2-devel] [PATCH] MdePkg/Include: Add HTTP definitions

gaoliming
 

Reviewed-by: Liming Gao <gaoliming@...>

-----邮件原件-----
发件人: bounce+27952+65675+4905953+8761045@groups.io
<bounce+27952+65675+4905953+8761045@groups.io> 代表 Abner Chang
发送时间: 2020年9月28日 14:01
收件人: devel@edk2.groups.io
抄送: abner.chang@...; Michael D Kinney
<michael.d.kinney@...>; Liming Gao <gaoliming@...>;
Zhiguang Liu <zhiguang.liu@...>; Wu Jiaxin <jiaxin.wu@...>; Fu
Siyuan <siyuan.fu@...>; Wang Fan <fan.wang@...>; Nickle
Wang <nickle.wang@...>; Maciej Rabeda
<maciej.rabeda@...>
主题: [edk2-devel] [PATCH] MdePkg/Include: Add HTTP definitions

BZ #2915,
https://bugzilla.tianocore.org/show_bug.cgi?id=2915

Add definitions for HTTP chunk transfer. HTTP chunk
would be used in UEFI Redfish solution for provisioning
or patch BIOS Redfish properties.

Signed-off-by: Abner Chang <abner.chang@...>

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Cc: Wu Jiaxin <jiaxin.wu@...>
Cc: Fu Siyuan <siyuan.fu@...>
Cc: Wang Fan <fan.wang@...>
Cc: Nickle Wang <nickle.wang@...>
Cc: Maciej Rabeda <maciej.rabeda@...>
---
MdePkg/Include/IndustryStandard/Http11.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/IndustryStandard/Http11.h
b/MdePkg/Include/IndustryStandard/Http11.h
index d124bec37b0..890bb6913bd 100644
--- a/MdePkg/Include/IndustryStandard/Http11.h
+++ b/MdePkg/Include/IndustryStandard/Http11.h
@@ -146,7 +146,11 @@
/// is a property of the message, not of the entity.
///
#define HTTP_HEADER_TRANSFER_ENCODING "Transfer-Encoding"
-
+#define HTTP_HEADER_TRANSFER_ENCODING_CHUNKED "chunked"
+#define CHUNKED_TRNASFER_CODING_CR '\r'
+#define CHUNKED_TRNASFER_CODING_LF '\n'
+#define CHUNKED_TRNASFER_CODING_LAST_CHUNK '0'
+#define CHUNKED_TRNASFER_CODING_EXTENSION_SAPERATOR ';'

///
/// User Agent Request Header
--
2.21.0.windows.1





回复: [edk2-devel] [Patch V4 1/1] [BUG FIX] Tools\FitGen: FIX for GCC Build Failure

gaoliming
 

Reviewed-by: Liming Gao <gaoliming@...>

Merge @ b0459fb6cf5083f5fcdf91d1e1518a86b22add1b in edk2-platform.

Thanks
Liming

-----邮件原件-----
发件人: bounce+27952+66168+4905953+8761045@groups.io
<bounce+27952+66168+4905953+8761045@groups.io> 代表 Bob Feng
发送时间: 2020年10月13日 17:00
收件人: Duggapu, Chinni B <chinni.b.duggapu@...>;
devel@edk2.groups.io
抄送: Liming Gao <gaoliming@...>; Liu, Zhiguang
<zhiguang.liu@...>
主题: Re: [edk2-devel] [Patch V4 1/1] [BUG FIX] Tools\FitGen: FIX for GCC
Build Failure

Reviewed-by: Bob Feng <bob.c.feng@...>

-----Original Message-----
From: Duggapu, Chinni B <chinni.b.duggapu@...>
Sent: Tuesday, October 13, 2020 1:30 PM
To: devel@edk2.groups.io
Cc: Duggapu, Chinni B <chinni.b.duggapu@...>; Feng, Bob C
<bob.c.feng@...>; Liming Gao <gaoliming@...>; Liu; Liu,
Zhiguang <zhiguang.liu@...>
Subject: [Patch V4 1/1] [BUG FIX] Tools\FitGen: FIX for GCC Build Failure

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

Added Type Casting to print UINT64 Value using Printf to fix the GCC build
error.

Signed-off-by: cbduggap <chinni.b.duggapu@...>
Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Cc: Liu, Zhiguang <zhiguang.liu@...>

Signed-off-by: cbduggap <chinni.b.duggapu@...>
---
Silicon/Intel/Tools/FitGen/FitGen.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Intel/Tools/FitGen/FitGen.c
b/Silicon/Intel/Tools/FitGen/FitGen.c
index febeb008fe..e9541c1e95 100644
--- a/Silicon/Intel/Tools/FitGen/FitGen.c
+++ b/Silicon/Intel/Tools/FitGen/FitGen.c
@@ -1004,7 +1004,7 @@ Returns:
//

gFitTableContext.TopFlashAddressRemapValue = 0x100000000;

}

- printf ("Top Flash Address Value : 0x%llx\n",
gFitTableContext.TopFlashAddressRemapValue);

+ printf ("Top Flash Address Value : 0x%llx\n", (unsigned long long)
gFitTableContext.TopFlashAddressRemapValue);

//

// 0.4 Clear FIT table related memory

//

--
2.26.2.windows.1





回复: [edk2-devel] [edk2-staging][PATCH v2] BaseTools/Fmmt: Fixed FMMT Linux build break issue

gaoliming
 

Reviewed-by: Liming Gao <gaoliming@...>

-----邮件原件-----
发件人: bounce+27952+66201+4905953+8761045@groups.io
<bounce+27952+66201+4905953+8761045@groups.io> 代表 Bob Feng
发送时间: 2020年10月14日 11:11
收件人: Yunhua Feng <fengyunhua@...>; devel@edk2.groups.io
抄送: Liming Gao <gaoliming@...>
主题: Re: [edk2-devel] [edk2-staging][PATCH v2] BaseTools/Fmmt: Fixed
FMMT Linux build break issue

This patch looks good.

Reviewed-by: Bob Feng <bob.c.feng@...>

-----Original Message-----
From: Yunhua Feng <fengyunhua@...>
Sent: Wednesday, October 14, 2020 10:49 AM
To: devel@edk2.groups.io
Cc: Feng, Bob C <bob.c.feng@...>; Liming Gao
<gaoliming@...>
Subject: [edk2-staging][PATCH v2] BaseTools/Fmmt: Fixed FMMT Linux build
break issue

Remove some defined variable but not used.
Linux build option[-Werror=unused-but-set-variable] treat all warnings as
errors.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>

Signed-off-by: Yunhua Feng <fengyunhua@...>
---
BaseTools/Source/C/FMMT/FirmwareModuleManagement.c | 14
++------------
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
index 20663ba163..38056153fb 100644
--- a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
+++ b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
@@ -1717,11 +1717,6 @@ FmmtImageExtract (
FV_INFORMATION *FvInFd;
UINT32 Index;
UINT32 FfsFoundFlag;
- FFS_INFORMATION *OutputFileName;
- FILE* NewFdFile;
- FILE* NewFvFile;
- UINT64 NewFvLength;
- VOID* Buffer;
CHAR8 *TemDir;
UINT8 FvNumInFd;
UINT32 Offset;
@@ -1736,23 +1731,18 @@ FmmtImageExtract (
UINT32 FfsSize;
UINT32 FdSize;
int j;
- CHAR8 FfsOutputFileName[_MAX_DIR];
+ CHAR8 FfsOutputFileName[_MAX_PATH];

FdSize = 0;
Index = 0;
- NewFvLength = 0;
FfsFoundFlag = 0;
FdData = NULL;
FvInFd = NULL;
- OutputFileName = NULL;
- NewFdFile = NULL;
- NewFvFile = NULL;
- Buffer = NULL;
TemDir = NULL;
FvNumInFd = 0;
Offset = 0;
FdBuffer = NULL;
- if (sizeof(FfsOutFileOrDirName) > _MAX_DIR) {
+ if (strlen(FfsOutFileOrDirName) > _MAX_PATH - 1) {
Error("FMMT", 0, 0004, "error while input file name", "Output
directory
path is too long" );
return EFI_ABORTED;
}
--
2.27.0.windows.1






Re: [edk2-staging][PATCH v2] BaseTools/Fmmt: Fixed FMMT Linux build break issue

Bob Feng
 

This patch looks good.

Reviewed-by: Bob Feng <bob.c.feng@...>

-----Original Message-----
From: Yunhua Feng <fengyunhua@...>
Sent: Wednesday, October 14, 2020 10:49 AM
To: devel@edk2.groups.io
Cc: Feng, Bob C <bob.c.feng@...>; Liming Gao <gaoliming@...>
Subject: [edk2-staging][PATCH v2] BaseTools/Fmmt: Fixed FMMT Linux build break issue

Remove some defined variable but not used.
Linux build option[-Werror=unused-but-set-variable] treat all warnings as errors.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>

Signed-off-by: Yunhua Feng <fengyunhua@...>
---
BaseTools/Source/C/FMMT/FirmwareModuleManagement.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
index 20663ba163..38056153fb 100644
--- a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
+++ b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
@@ -1717,11 +1717,6 @@ FmmtImageExtract (
FV_INFORMATION *FvInFd;
UINT32 Index;
UINT32 FfsFoundFlag;
- FFS_INFORMATION *OutputFileName;
- FILE* NewFdFile;
- FILE* NewFvFile;
- UINT64 NewFvLength;
- VOID* Buffer;
CHAR8 *TemDir;
UINT8 FvNumInFd;
UINT32 Offset;
@@ -1736,23 +1731,18 @@ FmmtImageExtract (
UINT32 FfsSize;
UINT32 FdSize;
int j;
- CHAR8 FfsOutputFileName[_MAX_DIR];
+ CHAR8 FfsOutputFileName[_MAX_PATH];

FdSize = 0;
Index = 0;
- NewFvLength = 0;
FfsFoundFlag = 0;
FdData = NULL;
FvInFd = NULL;
- OutputFileName = NULL;
- NewFdFile = NULL;
- NewFvFile = NULL;
- Buffer = NULL;
TemDir = NULL;
FvNumInFd = 0;
Offset = 0;
FdBuffer = NULL;
- if (sizeof(FfsOutFileOrDirName) > _MAX_DIR) {
+ if (strlen(FfsOutFileOrDirName) > _MAX_PATH - 1) {
Error("FMMT", 0, 0004, "error while input file name", "Output directory path is too long" );
return EFI_ABORTED;
}
--
2.27.0.windows.1


[edk2-staging][PATCH v2] BaseTools/Fmmt: Fixed FMMT Linux build break issue

fengyunhua
 

Remove some defined variable but not used.
Linux build option[-Werror=unused-but-set-variable] treat all warnings as errors.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>

Signed-off-by: Yunhua Feng <fengyunhua@...>
---
BaseTools/Source/C/FMMT/FirmwareModuleManagement.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
index 20663ba163..38056153fb 100644
--- a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
+++ b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
@@ -1717,11 +1717,6 @@ FmmtImageExtract (
FV_INFORMATION *FvInFd;
UINT32 Index;
UINT32 FfsFoundFlag;
- FFS_INFORMATION *OutputFileName;
- FILE* NewFdFile;
- FILE* NewFvFile;
- UINT64 NewFvLength;
- VOID* Buffer;
CHAR8 *TemDir;
UINT8 FvNumInFd;
UINT32 Offset;
@@ -1736,23 +1731,18 @@ FmmtImageExtract (
UINT32 FfsSize;
UINT32 FdSize;
int j;
- CHAR8 FfsOutputFileName[_MAX_DIR];
+ CHAR8 FfsOutputFileName[_MAX_PATH];

FdSize = 0;
Index = 0;
- NewFvLength = 0;
FfsFoundFlag = 0;
FdData = NULL;
FvInFd = NULL;
- OutputFileName = NULL;
- NewFdFile = NULL;
- NewFvFile = NULL;
- Buffer = NULL;
TemDir = NULL;
FvNumInFd = 0;
Offset = 0;
FdBuffer = NULL;
- if (sizeof(FfsOutFileOrDirName) > _MAX_DIR) {
+ if (strlen(FfsOutFileOrDirName) > _MAX_PATH - 1) {
Error("FMMT", 0, 0004, "error while input file name", "Output directory path is too long" );
return EFI_ABORTED;
}
--
2.27.0.windows.1


Re: [PATCH] IntelFsp2Pkg/Tools: Fix a typo issue

Nate DeSimone
 

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@...>

On 10/12/20, 7:44 PM, "fengyunhua" <fengyunhua@...> wrote:

Error message:
raise Exception ("'%s' is not a valid directory!" % FvDir)
NameError: name 'FvDir' is not defined

FvDir should be fvDir.

Cc: Chasel Chiu <chasel.chiu@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Star Zeng <star.zeng@...>
Signed-off-by: Yunhua Feng <fengyunhua@...>
---
IntelFsp2Pkg/Tools/PatchFv.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/IntelFsp2Pkg/Tools/PatchFv.py b/IntelFsp2Pkg/Tools/PatchFv.py
index edb30c816b..0c8d908063 100644
--- a/IntelFsp2Pkg/Tools/PatchFv.py
+++ b/IntelFsp2Pkg/Tools/PatchFv.py
@@ -163,7 +163,7 @@ class Symbols:
# If the fvDir is not a directory, then raise an exception
#
if not os.path.isdir(fvDir):
- raise Exception ("'%s' is not a valid directory!" % FvDir)
+ raise Exception ("'%s' is not a valid directory!" % fvDir)

#
# If the Guid.xref is not existing in fvDir, then raise an exception
--
2.27.0.windows.1


Query regarding build error

Bhavsar, Samprat <samprat.bhavsar@...>
 

Hi,

I’m a new intern and I was trying to add a library in the DCG10nm project which lead to a fatal error while building. Please can you help me out for finding and resolving the issue. PFA of build log file.

 

 

 

Thanks and Regards,

Samprat

 


Re: 答复: [edk2-devel] OpensslLib compiling error : cannot open include file : openssl/aes.h

Tiger Liu(BJ-RD)
 

Hi, Liming:
From Build.log, it seems include path is ok:
......
cl.exe" /Foy:\Build\ZxxPlatformPkg\DEBUG_VS2015x86\X64\CryptoPkg\Library\BaseCryptLib\RuntimeCryptLib\OUTPUT\Hash\ /showIncludes /nologo /c /GS- /W4 /Gs32768 /D UNICODE /O1b2s /GL /Gy /FIAutoGen.h /EHs-c- /GR- /GF /Z7 /Gw /wd4090 -
......
/Iy:\CryptoPkg /Iy:\CryptoPkg\Include /Iy:\CryptoPkg\Private /Iy:\CryptoPkg\Library\Include /Iy:\CryptoPkg\Library\OpensslLib\openssl\include y:\CryptoPkg\Library\BaseCryptLib\Hash\CryptSm3.c y:\CryptoPkg\Library\BaseCryptLib\Hash\CryptMd5.c y:\CryptoPkg\Library\BaseCryptLib\Hash\CryptSha256.c y:\CryptoPkg\Library\BaseCryptLib\Hash\CryptSha512Null.c y:\CryptoPkg\Library\BaseCryptLib\Hash\CryptSha1.c

And BaseCryptLib / IntrinsicLib compiled successfully.

And I found a cc_resp.txt file in Build\...\DEBUG_VS2015x86\X64\CryptoPkg\Library\OpensslLib\OpensslLib\OUTPUT
Why produce a cc_resp.txt?

Thanks
-----邮件原件-----
发件人: gaoliming <gaoliming@...>
发送时间: 2020年10月13日 18:03
收件人: devel@edk2.groups.io; Tiger Liu(BJ-RD) <TigerLiu@...>
主题: 回复: [edk2-devel] OpensslLib compiling error : cannot open include file : openssl/aes.h

Can you print the full build log and check the compiler include path?

Is this a clean build issue or incremental build issue?

Thanks
Liming
-----邮件原件-----
发件人: bounce+27952+66166+4905953+8761045@groups.io
<bounce+27952+66166+4905953+8761045@groups.io> 代表 Tiger
Liu(BJ-RD)
发送时间: 2020年10月13日 16:38
收件人: devel@edk2.groups.io
主题: [edk2-devel] OpensslLib compiling error : cannot open include file :
openssl/aes.h

Hi, experts:
When I compiled OpensslLib, I met a problem:
......
\CryptoPkg\Library\OpensslLib\openssl\crypto\aes\aes_core.c(42): fatal
error C1083: Cannot open include file: 'openssl/crypto.h': No such
file or directory
\CryptoPkg\Library\OpensslLib\openssl\crypto\aes\aes_misc.c(10): fatal
error C1083: Cannot open include file: 'openssl/opensslv.h': No such
file or directory ......

But these files exist in
CryptoPkg\Library\OpensslLib\openssl\include\openssl .

Does anybody meet same problems?

Note:
CryptoPkg.dec has also declared this include path:
......
[Includes.Common.Private]
Private
Library/Include
Library/OpensslLib/openssl/include
......

Thanks


保密声明:
本邮件含有保密或专有信息,仅供指定收件人使用。严禁对本邮件或其
内容做任何未经授权的查阅、使用、复制或转发。
CONFIDENTIAL NOTE:
This email contains confidential or legally privileged information and
is for the sole use of its intended recipient. Any unauthorized
review, use, copying or forwarding of this email or the content of this email is strictly prohibited.







保密声明:
本邮件含有保密或专有信息,仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。
CONFIDENTIAL NOTE:
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.


Re: [PATCH v3 1/1] ShellPkg/AcpiView: HMAT Parser

Gao, Zhichao
 

The patch looks good to me. Before take my R-B, have you tested the patch. It is better to get another Tested-by.
By the way, it is highly recommended to add one BZ to descript the change and keep a good history.

Thanks,
Zhichao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami
Mujawar
Sent: Wednesday, September 23, 2020 2:01 AM
To: devel@edk2.groups.io
Cc: Sami Mujawar <sami.mujawar@...>; Ni, Ray <ray.ni@...>; Gao,
Zhichao <zhichao.gao@...>; Matteo.Carlini@...;
Ben.Adderson@...; nd@...
Subject: [edk2-devel] [PATCH v3 1/1] ShellPkg/AcpiView: HMAT Parser

From: Marc Moisson-Franckhauser <marc.moisson-franckhauser@...>

Add a new parser for the Heterogeneous Memory Attribute Table. The parser
also validates some fields for this table.

The HMAT table is used to describe the memory attributes such as memory side
cache attributes and bandwidth and latency details related to memory proximity
domains. The info in the HMAT table can be used by an operating system for
optimisation.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-
franckhauser@...>
Signed-off-by: Sami Mujawar <sami.mujawar@...>
---

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/833_hmat_parser_v3

Notes:
v3:
- Updates based on review comments, minor updates to output [SAMI]
formatting and added additional checks.

v2:
- Fixed minor output formatting in DumpCacheAttributes() [SAMI]

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 28 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h | 4
+-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
| 650 ++++++++++++++++++++

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
| 3 +-

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.i
nf | 3 +-

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.
uni | 3 +-
6 files changed, 685 insertions(+), 6 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index
f81ccac7e118378aa185db4b625e5bcd75f78347..969cc0b371852f01f30c88dc506
374a459c9c19e 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -1,7 +1,7 @@
/** @file
Header file for ACPI parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent **/

@@ -592,6 +592,32 @@ ParseAcpiGtdt (
);

/**
+ This function parses the ACPI HMAT table.
+ When trace is enabled this function parses the HMAT table and traces
+ the ACPI table fields.
+
+ This function parses the following HMAT structures:
+ - Memory Proximity Domain Attributes Structure (Type 0)
+ - System Locality Latency and Bandwidth Info Structure (Type 1)
+ - Memory Side Cache Info structure (Type 2)
+
+ This function also performs validation of the ACPI table fields.
+
+ @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] Ptr Pointer to the start of the buffer.
+ @param [in] AcpiTableLength Length of the ACPI table.
+ @param [in] AcpiTableRevision Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiHmat (
+ IN BOOLEAN Trace,
+ IN UINT8* Ptr,
+ IN UINT32 AcpiTableLength,
+ IN UINT8 AcpiTableRevision
+ );
+
+/**
This function parses the ACPI IORT table.
When trace is enabled this function parses the IORT table and
traces the ACPI fields.
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
index
4f92596b90a6ee422d8d0959881015ffd3de4da0..f8e8b5979f3be041bbc8d17042
b2db8e0b73f205 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
@@ -1,7 +1,7 @@
/** @file
Header file for ACPI table parser

- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2018, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent **/

@@ -11,7 +11,7 @@
/**
The maximum number of ACPI table parsers.
*/
-#define MAX_ACPI_TABLE_PARSERS 16
+#define MAX_ACPI_TABLE_PARSERS 32

/** An invalid/NULL signature value.
*/
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
new file mode 100644
index
0000000000000000000000000000000000000000..4cbbb64bc7d97b9896269ca18
2a7cf4916a0a43e
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatPars
+++ er.c
@@ -0,0 +1,650 @@
+/** @file
+ HMAT table parser
+
+ Copyright (c) 2020, Arm Limited.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Reference(s):
+ - ACPI 6.3 Specification - January 2019
+
+ @par Glossary:
+ - MPDA - Memory Proximity Domain Attributes
+ - SLLBI - System Locality Latency and Bandwidth Information
+ - MSCI - Memory Side Cache Information
+ - Dom - Domain
+**/
+
+#include <Library/PrintLib.h>
+#include <Library/BaseLib.h>
+#include <Library/UefiLib.h>
+#include "AcpiParser.h"
+#include "AcpiView.h"
+
+// Maximum Memory Domain matrix print size.
+#define MAX_MEMORY_DOMAIN_TARGET_PRINT_MATRIX 10
+
+// Local variables
+STATIC CONST UINT16* HmatStructureType; STATIC CONST UINT32*
+HmatStructureLength;
+
+STATIC CONST UINT32* NumberInitiatorProximityDomain; STATIC CONST
+UINT32* NumberTargetProximityDomain; STATIC CONST
+EFI_ACPI_6_3_HMAT_STRUCTURE_SYSTEM_LOCALITY_LATENCY_AND_BAND
WIDTH_INFO_
+FLAGS*
+SllbiFlags;
+
+STATIC CONST UINT8* SllbiDataType;
+STATIC CONST UINT32* NumberSMBIOSHandles;
+
+STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
+
+/**
+ Names of System Locality Latency Bandwidth Information (SLLBI) data
+types **/ STATIC CONST CHAR16* SllbiNames[] = {
+ L"Access %sLatency%s",
+ L"Read %sLatency%s",
+ L"Write %sLatency%s",
+ L"Access %sBandwidth%s",
+ L"Read %sBandwidth%s",
+ L"Write %sBandwidth%s"
+};
+
+/**
+ This function validates the Cache Attributes field.
+
+ @param [in] Ptr Pointer to the start of the field data.
+ @param [in] Context Pointer to context specific information e.g. this
+ could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateCacheAttributes (
+ IN UINT8* Ptr,
+ IN VOID* Context
+ )
+{
+
EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTR
IBUTES*
+ Attributes;
+
+ Attributes =
+
+
(EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATT
RIBUTES*)
+ Ptr;
+
+ if (Attributes->TotalCacheLevels > 0x3) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: Attributes bits [3:0] have invalid value: 0x%x",
+ Attributes->TotalCacheLevels
+ );
+ }
+ if (Attributes->CacheLevel > 0x3) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: Attributes bits [7:4] have invalid value: 0x%x",
+ Attributes->CacheLevel
+ );
+ }
+ if (Attributes->CacheAssociativity > 0x2) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: Attributes bits [11:8] have invalid value: 0x%x",
+ Attributes->CacheAssociativity
+ );
+ }
+ if (Attributes->WritePolicy > 0x2) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: Attributes bits [15:12] have invalid value: 0x%x",
+ Attributes->WritePolicy
+ );
+ }
+}
+
+/**
+ Dumps the cache attributes field
+
+ @param [in] Format Optional format string for tracing the data.
+ @param [in] Ptr Pointer to the start of the buffer.
+**/
+STATIC
+VOID
+EFIAPI
+DumpCacheAttributes (
+ IN CONST CHAR16* Format OPTIONAL,
+ IN UINT8* Ptr
+ )
+{
+
EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTR
IBUTES*
+ Attributes;
+
+ Attributes =
+
+
(EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATT
RIBUTES*)
+ Ptr;
+
+ Print (L"\n");
+ PrintFieldName (4, L"Total Cache Levels");
+ Print (L"%d\n", Attributes->TotalCacheLevels);
+ PrintFieldName (4, L"Cache Level");
+ Print (L"%d\n", Attributes->CacheLevel);
+ PrintFieldName (4, L"Cache Associativity");
+ Print (L"%d\n", Attributes->CacheAssociativity);
+ PrintFieldName (4, L"Write Policy");
+ Print (L"%d\n", Attributes->WritePolicy);
+ PrintFieldName (4, L"Cache Line Size");
+ Print (L"%d\n", Attributes->CacheLineSize); }
+
+/**
+ An ACPI_PARSER array describing the ACPI HMAT Table.
+*/
+STATIC CONST ACPI_PARSER HmatParser[] = {
+ PARSE_ACPI_HEADER (&AcpiHdrInfo),
+ {L"Reserved", 4, 36, NULL, NULL, NULL, NULL, NULL} };
+
+/**
+ An ACPI_PARSER array describing the HMAT structure header.
+*/
+STATIC CONST ACPI_PARSER HmatStructureHeaderParser[] = {
+ {L"Type", 2, 0, NULL, NULL, (VOID**)&HmatStructureType, NULL, NULL},
+ {L"Reserved", 2, 2, NULL, NULL, NULL, NULL, NULL},
+ {L"Length", 4, 4, NULL, NULL, (VOID**)&HmatStructureLength, NULL,
+NULL} };
+
+/**
+ An ACPI PARSER array describing the Memory Proximity Domain
+Attributes
+ Structure - Type 0.
+*/
+STATIC CONST ACPI_PARSER MemProximityDomainAttributeParser[] = {
+ {L"Type", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Length", 4, 4, L"%d", NULL, NULL, NULL, NULL},
+ {L"Flags", 2, 8, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 2, 10, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Proximity Dom for initiator", 4, 12, L"0x%x", NULL, NULL, NULL,
+NULL},
+ {L"Proximity Dom for memory", 4, 16, L"0x%x", NULL, NULL, NULL,
+NULL},
+ {L"Reserved", 4, 20, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 8, 24, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Reserved", 8, 32, L"0x%lx", NULL, NULL, NULL, NULL} };
+
+/**
+ An ACPI PARSER array describing the System Locality Latency and
+Bandwidth
+ Information Structure - Type 1.
+*/
+STATIC CONST ACPI_PARSER SllbiParser[] = {
+ {L"Type", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Length", 4, 4, L"%d", NULL, NULL, NULL, NULL},
+ {L"Flags", 1, 8, L"0x%x", NULL, (VOID**)&SllbiFlags, NULL, NULL},
+ {L"Data type", 1, 9, L"0x%x", NULL, (VOID**)&SllbiDataType, NULL,
+NULL},
+ {L"Reserved", 2, 10, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Initiator Proximity Dom Count", 4, 12, L"%d", NULL,
+ (VOID**)&NumberInitiatorProximityDomain, NULL, NULL},
+ {L"Target Proximity Dom Count", 4, 16, L"%d", NULL,
+ (VOID**)&NumberTargetProximityDomain, NULL, NULL},
+ {L"Reserved", 4, 20, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Entry Base Unit", 8, 24, L"0x%lx", NULL, NULL, NULL, NULL}
+ // initiator Proximity Domain list ...
+ // target Proximity Domain list ...
+ // Latency/Bandwidth matrix ...
+};
+
+/**
+ An ACPI PARSER array describing the Memory Side Cache Information
+ Structure - Type 2.
+*/
+STATIC CONST ACPI_PARSER MemSideCacheInfoParser[] = {
+ {L"Type", 2, 0, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Length", 4, 4, L"%d", NULL, NULL, NULL, NULL},
+ {L"Proximity Dom for memory", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Memory Side Cache Size", 8, 16, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Cache Attributes", 4, 24, NULL, DumpCacheAttributes, NULL,
+ ValidateCacheAttributes, NULL},
+ {L"Reserved", 2, 28, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"SMBIOS Handle Count", 2, 30, L"%d", NULL,
+ (VOID**)&NumberSMBIOSHandles, NULL, NULL}
+ // SMBIOS handles List ...
+};
+
+/**
+ This function parses the Memory Proximity Domain Attributes
+ Structure (Type 0).
+
+ @param [in] Ptr Pointer to the start of the Memory Proximity Domain
+ Attributes Structure data.
+ @param [in] Length Length of the Memory Proximity Domain Attributes
+ Structure.
+**/
+STATIC
+VOID
+DumpMpda (
+ IN UINT8* Ptr,
+ IN UINT32 Length
+ )
+{
+ ParseAcpi (
+ TRUE,
+ 2,
+ "Memory Proximity Domain Attributes Structure",
+ Ptr,
+ Length,
+ PARSER_PARAMS (MemProximityDomainAttributeParser)
+ );
+}
+
+/**
+ This function parses the System Locality Latency and Bandwidth
+Information
+ Structure (Type 1).
+
+ @param [in] Ptr Pointer to the start of the System Locality Latency and
+ Bandwidth Information Structure data.
+ @param [in] Length Length of the System Locality Latency and Bandwidth
+ Information Structure.
+**/
+STATIC
+VOID
+DumpSllbi (
+ IN UINT8* Ptr,
+ IN UINT32 Length
+ )
+{
+ CONST UINT32* InitiatorProximityDomainList;
+ CONST UINT32* TargetProximityDomainList;
+ CONST UINT16* LatencyBandwidthMatrix;
+ UINT32 Offset;
+ CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
+ CHAR16 SecondBuffer[OUTPUT_FIELD_COLUMN_WIDTH];
+ UINT32 RequiredTableSize;
+ UINT32 Index;
+ UINT32 IndexInitiator;
+ UINT32 IndexTarget;
+ UINT32 TargetStartOffset;
+
+ Offset = ParseAcpi (
+ TRUE,
+ 2,
+ "System Locality Latency and Bandwidth Information Structure",
+ Ptr,
+ Length,
+ PARSER_PARAMS (SllbiParser)
+ );
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((SllbiFlags == NULL) ||
+ (SllbiDataType == NULL) ||
+ (NumberInitiatorProximityDomain == NULL) ||
+ (NumberTargetProximityDomain == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"SLLBI structure header. Length = %d.\n",
+ Length
+ );
+ return;
+ }
+
+ RequiredTableSize = (*NumberInitiatorProximityDomain * sizeof (UINT32)) +
+ (*NumberTargetProximityDomain * sizeof (UINT32)) +
+ (*NumberInitiatorProximityDomain *
+ *NumberTargetProximityDomain * sizeof (UINT16)) +
+ Offset;
+
+ if (RequiredTableSize > Length) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient System Locality Latency and Bandwidth" \
+ L"Information Structure length. TableLength = %d. " \
+ L"RequiredTableLength = %d.\n",
+ Length,
+ RequiredTableSize
+ );
+ return;
+ }
+
+ InitiatorProximityDomainList = (UINT32*) (Ptr + Offset);
+ TargetProximityDomainList = InitiatorProximityDomainList +
+ *NumberInitiatorProximityDomain;
+ LatencyBandwidthMatrix = (UINT16*) (TargetProximityDomainList +
+ *NumberTargetProximityDomain);
+
+ // Display each element of the Initiator Proximity Domain list for
+ (Index = 0; Index < *NumberInitiatorProximityDomain; Index++) {
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ L"Initiator Proximity Dom [%d]",
+ Index
+ );
+
+ PrintFieldName (4, Buffer);
+ Print (
+ L"0x%x\n",
+ InitiatorProximityDomainList[Index]
+ );
+ }
+
+ // Display each element of the Target Proximity Domain list for
+ (Index = 0; Index < *NumberTargetProximityDomain; Index++) {
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ L"Target Proximity Dom [%d]",
+ Index
+ );
+
+ PrintFieldName (4, Buffer);
+ Print (
+ L"0x%x\n",
+ TargetProximityDomainList[Index]
+ );
+ }
+
+ // Create base name depending on Data Type in this Structure if
+ (*SllbiDataType >= ARRAY_SIZE (SllbiNames)) {
+ IncrementErrorCount ();
+ Print (L"Error: Unkown Data Type. DataType = 0x%x.\n", *SllbiDataType);
+ return;
+ }
+ StrCpyS (Buffer, sizeof (Buffer), SllbiNames[*SllbiDataType]);
+
+ // Adjust base name depending on Memory Hierarchy in this Structure
+ switch (SllbiFlags->MemoryHierarchy) {
+ case 0:
+ UnicodeSPrint (
+ SecondBuffer,
+ sizeof (SecondBuffer),
+ Buffer,
+ L"",
+ L"%s"
+ );
+ break;
+ case 1:
+ case 2:
+ case 3:
+ UnicodeSPrint (
+ SecondBuffer,
+ sizeof (SecondBuffer),
+ Buffer,
+ L"Hit ",
+ L"%s"
+ );
+ break;
+ default:
+ IncrementErrorCount ();
+ Print (
+ L"Error: Invalid Memory Hierarchy. MemoryHierarchy = %d.\n",
+ SllbiFlags->MemoryHierarchy
+ );
+ return;
+
+ } // switch
+
+ if (*NumberTargetProximityDomain <=
MAX_MEMORY_DOMAIN_TARGET_PRINT_MATRIX) {
+ // Display the latency/bandwidth matrix as a matrix
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ SecondBuffer,
+ L""
+ );
+ PrintFieldName (4, Buffer);
+
+ Print (L"\n Target : X-axis (Horizontal)");
+ Print (L"\n Initiator : Y-axis (Vertical)");
+ Print (L"\n |");
+
+ for (IndexTarget = 0;
+ IndexTarget < *NumberTargetProximityDomain;
+ IndexTarget++) {
+ Print (L" %2d", IndexTarget);
+ }
+
+ Print (L"\n ---+");
+ for (IndexTarget = 0;
+ IndexTarget < *NumberTargetProximityDomain;
+ IndexTarget++) {
+ Print (L"------");
+ }
+ Print (L"\n");
+
+ TargetStartOffset = 0;
+ for (IndexInitiator = 0;
+ IndexInitiator < *NumberInitiatorProximityDomain;
+ IndexInitiator++) {
+ Print (L" %2d |", IndexInitiator);
+ for (IndexTarget = 0;
+ IndexTarget < *NumberTargetProximityDomain;
+ IndexTarget++) {
+ Print (
+ L" %5d",
+ LatencyBandwidthMatrix[TargetStartOffset + IndexTarget]
+ );
+ } // for Target
+ Print (L"\n");
+ TargetStartOffset += (*NumberTargetProximityDomain);
+ } // for Initiator
+ Print (L"\n");
+ } else {
+ // Display the latency/bandwidth matrix as a list
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ SecondBuffer,
+ L" [%d][%d]"
+ );
+
+ TargetStartOffset = 0;
+ for (IndexInitiator = 0;
+ IndexInitiator < *NumberInitiatorProximityDomain;
+ IndexInitiator++) {
+ for (IndexTarget = 0;
+ IndexTarget < *NumberTargetProximityDomain;
+ IndexTarget++) {
+ UnicodeSPrint (
+ SecondBuffer,
+ sizeof (SecondBuffer),
+ Buffer,
+ IndexInitiator,
+ IndexTarget
+ );
+
+ PrintFieldName (4, SecondBuffer);
+ Print (
+ L"%d\n",
+ LatencyBandwidthMatrix[TargetStartOffset + IndexTarget]
+ );
+ } // for Target
+ TargetStartOffset += (*NumberTargetProximityDomain);
+ } // for Initiator
+ }
+}
+
+/**
+ This function parses the Memory Side Cache Information Structure (Type 2).
+
+ @param [in] Ptr Pointer to the start of the Memory Side Cache Information
+ Structure data.
+ @param [in] Length Length of the Memory Side Cache Information Structure.
+**/
+STATIC
+VOID
+DumpMsci (
+ IN UINT8* Ptr,
+ IN UINT32 Length
+ )
+{
+ CONST UINT16* SMBIOSHandlesList;
+ CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
+ UINT32 Offset;
+ UINT16 Index;
+
+ Offset = ParseAcpi (
+ TRUE,
+ 2,
+ "Memory Side Cache Information Structure",
+ Ptr,
+ Length,
+ PARSER_PARAMS (MemSideCacheInfoParser)
+ );
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if (NumberSMBIOSHandles == NULL) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"MSCI structure header. Length = %d.\n",
+ Length
+ );
+ return;
+ }
+
+ if ((*NumberSMBIOSHandles * sizeof (UINT16)) > (Length - Offset)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Number of SMBIOS Handles. SMBIOSHandlesCount = %d."
\
+ L"RemainingBufferLength = %d.\n",
+ *NumberSMBIOSHandles,
+ Length - Offset
+ );
+ return;
+ }
+
+ SMBIOSHandlesList = (UINT16*) (Ptr + Offset);
+
+ for (Index = 0; Index < *NumberSMBIOSHandles; Index++) {
+ UnicodeSPrint (
+ Buffer,
+ sizeof (Buffer),
+ L"SMBIOS Handles [%d]",
+ Index
+ );
+
+ PrintFieldName (4, Buffer);
+ Print (
+ L"0x%x\n",
+ SMBIOSHandlesList[Index]
+ );
+ }
+}
+
+/**
+ This function parses the ACPI HMAT table.
+ When trace is enabled this function parses the HMAT table and
+ traces the ACPI table fields.
+
+ This function parses the following HMAT structures:
+ - Memory Proximity Domain Attributes Structure (Type 0)
+ - System Locality Latency and Bandwidth Info Structure (Type 1)
+ - Memory Side Cache Info structure (Type 2)
+
+ This function also performs validation of the ACPI table fields.
+
+ @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] Ptr Pointer to the start of the buffer.
+ @param [in] AcpiTableLength Length of the ACPI table.
+ @param [in] AcpiTableRevision Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiHmat (
+ IN BOOLEAN Trace,
+ IN UINT8* Ptr,
+ IN UINT32 AcpiTableLength,
+ IN UINT8 AcpiTableRevision
+ )
+{
+ UINT32 Offset;
+ UINT8* HmatStructurePtr;
+
+ if (!Trace) {
+ return;
+ }
+
+ Offset = ParseAcpi (
+ Trace,
+ 0,
+ "HMAT",
+ Ptr,
+ AcpiTableLength,
+ PARSER_PARAMS (HmatParser)
+ );
+
+ HmatStructurePtr = Ptr + Offset;
+
+ while (Offset < AcpiTableLength) {
+ // Parse HMAT Structure Header to obtain Type and Length.
+ ParseAcpi (
+ FALSE,
+ 0,
+ NULL,
+ HmatStructurePtr,
+ AcpiTableLength - Offset,
+ PARSER_PARAMS (HmatStructureHeaderParser)
+ );
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((HmatStructureType == NULL) ||
+ (HmatStructureLength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"HMAT structure header. Length = %d.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
+ // Validate HMAT Structure length.
+ if ((*HmatStructureLength == 0) ||
+ ((Offset + (*HmatStructureLength)) > AcpiTableLength)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid HMAT Structure length. " \
+ L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
+ *HmatStructureLength,
+ Offset,
+ AcpiTableLength
+ );
+ return;
+ }
+
+ switch (*HmatStructureType) {
+ case
EFI_ACPI_6_3_HMAT_TYPE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES:
+ DumpMpda (
+ HmatStructurePtr,
+ *HmatStructureLength
+ );
+ break;
+ case
EFI_ACPI_6_3_HMAT_TYPE_SYSTEM_LOCALITY_LATENCY_AND_BANDWIDTH_I
NFO:
+ DumpSllbi (
+ HmatStructurePtr,
+ *HmatStructureLength
+ );
+ break;
+ case EFI_ACPI_6_3_HMAT_TYPE_MEMORY_SIDE_CACHE_INFO:
+ DumpMsci (
+ HmatStructurePtr,
+ *HmatStructureLength
+ );
+ break;
+ default:
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Unknown HMAT structure:"
+ L" Type = %d, Length = %d\n",
+ *HmatStructureType,
+ *HmatStructureLength
+ );
+ break;
+ } // switch
+
+ HmatStructurePtr += *HmatStructureLength;
+ Offset += *HmatStructureLength;
+ } // while
+}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.c
index
d2f26ff89f12e596702281c38ab0de3729aa68e4..c9e3054a5c413ba95e6420ce3d
02f0d954e99d90 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
+++ andLib.c
@@ -1,7 +1,7 @@
/** @file
Main file for 'acpiview' Shell command function.

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.<BR>
+ Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent **/

@@ -53,6 +53,7 @@ ACPI_TABLE_PARSER ParserList[] = {
{EFI_ACPI_6_3_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE,
ParseAcpiFacs},
{EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, ParseAcpiFadt},
{EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
ParseAcpiGtdt},
+ {EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_SIGNATURE,
+ ParseAcpiHmat},
{EFI_ACPI_6_2_IO_REMAPPING_TABLE_SIGNATURE, ParseAcpiIort},
{EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
ParseAcpiMadt},

{EFI_ACPI_6_2_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BA
SE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.inf
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.inf
index
91459f9ec632635ee453c5ef46f67445cd9eee0c..e970c4259c143b5b06fb1a759d5
819e7cb93e749 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.inf
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
+++ andLib.inf
@@ -1,7 +1,7 @@
## @file
# Provides Shell 'acpiview' command functions # -# Copyright (c) 2016 - 2020,
ARM Limited. All rights reserved.<BR>
+# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -33,6 +33,7 @@
[Sources.common]
Parsers/Facs/FacsParser.c
Parsers/Fadt/FadtParser.c
Parsers/Gtdt/GtdtParser.c
+ Parsers/Hmat/HmatParser.c
Parsers/Iort/IortParser.c
Parsers/Madt/MadtParser.c
Parsers/Madt/MadtParser.h
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.uni
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.uni
index
7cd43d0518fd0a23dc547a5cab0d08b62602a113..6b0537b47a751184e8a68a653
0aa85eb28ea33ba 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.uni
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
+++ andLib.uni
@@ -1,6 +1,6 @@
// /**
//
-// Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.<BR>
+// Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
// SPDX-License-Identifier: BSD-2-Clause-Patent // // Module Name:
@@ -84,6 +84,7 @@
" DSDT - Differentiated System Description Table\r\n"
" FACP - Fixed ACPI Description Table (FADT)\r\n"
" GTDT - Generic Timer Description Table\r\n"
+" HMAT - Heterogeneous Memory Attributes Table\r\n"
" IORT - IO Remapping Table\r\n"
" MCFG - Memory Mapped Config Space Base Address Description
Table\r\n"
" PPTT - Processor Properties Topology Table\r\n"
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'





TianoCore Bug Triage - APAC / NAMO - Tue, 10/13/2020 6:30pm-7:30pm #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Bug Triage - APAC / NAMO

When: Tuesday, 13 October 2020, 6:30pm to 7:30pm, (GMT-07:00) America/Los Angeles

Where:https://meetingsamer34.webex.com/meetingsamer34/j.php?MTID=m623b1dceab0c6dc62d56b2508fa50f39

View Event

Organizer: Liming Gao gaoliming@...

Description:

TianoCore Bug Triage - APAC / NAMO

Hosted by Liming Gao

 

Wednesday, Sep 30, 2020 9:30 am | 50 minutes | (UTC+08:00) Beijing, Chongqing, Hong Kong, Urumqi

Occurs every Tuesday effective 9/22/2020 from 6:30 PM to 7:20 PM, (UTC-07:00) Pacific Time (US & Canada)

Meeting number: 142 058 6312

Password: ESh5rcmvR37 (37457268 from video systems)

https://meetingsamer34.webex.com/meetingsamer34/j.php?MTID=m623b1dceab0c6dc62d56b2508fa50f39

 

Join by video system

Dial 1420586312@...

You can also dial 173.243.2.68 and enter your meeting number.

 

Join by phone

Use VoIP only


回复: [edk2-devel] [Patch] [edk2-staging]BaseTools/Fmmt: Fixed FMMT Linux build break issue

gaoliming
 

Got it. Here should be _MAX_PATH. Yunhua, can you fix this build issue on Linux?

Thanks
Liming

-----邮件原件-----
发件人: Feng, Bob C <bob.c.feng@...>
发送时间: 2020年10月13日 18:35
收件人: gaoliming <gaoliming@...>; devel@edk2.groups.io
抄送: 'Yunhua Feng' <fengyunhua@...>
主题: RE: [edk2-devel] [Patch] [edk2-staging]BaseTools/Fmmt: Fixed FMMT
Linux build break issue

Hi Liming,

What's purpose for the header file limits.h?
Bob: To get the Macro PATH_MAX definition. There is no _MAX_DIR or
_MAX_PATH Macro on Linux.

Can you use _MAX_PATH in the code to replace _MAX_DIR? _MAX_PATH is
defined here.
Bob: The original code use _MAX_DIR. It does not cause the build break. If
you and Yunhua confirm there should be _MAX_PATH, I can create another
patch to change the _MAX_DIR to _MAX_PATH

Thanks,
Bob

-----Original Message-----
From: gaoliming <gaoliming@...>
Sent: Tuesday, October 13, 2020 6:01 PM
To: devel@edk2.groups.io; Feng, Bob C <bob.c.feng@...>
Cc: 'Yunhua Feng' <fengyunhua@...>
Subject: 回复: [edk2-devel] [Patch] [edk2-staging]BaseTools/Fmmt: Fixed
FMMT Linux build break issue

Bob:
I add my comments.

Thanks
Liming
-----邮件原件-----
发件人: bounce+27952+66170+4905953+8761045@groups.io
<bounce+27952+66170+4905953+8761045@groups.io> 代表 Bob Feng
发送时间: 2020年10月13日 17:21
收件人: devel@edk2.groups.io
抄送: Yunhua Feng <fengyunhua@...>; Liming Gao
<gaoliming@...>
主题: [edk2-devel] [Patch] [edk2-staging]BaseTools/Fmmt: Fixed FMMT
Linux build break issue

Fixed the FMMT Linux build issue which was introduced by the commit
950333853b5fe2b73a7b5148501458cc97a01481

Signed-off-by: Bob Feng <bob.c.feng@...>
Cc: Yunhua Feng <fengyunhua@...>
Cc: Liming Gao <gaoliming@...>
---
.../Source/C/FMMT/FirmwareModuleManagement.c | 15
++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
index 20663ba163..8a7ae096d0 100644
--- a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
+++ b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
@@ -9,11 +9,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#include "FirmwareModuleManagement.h"
#include "Rebase.h"
#include <stdlib.h>
#include <wchar.h>
-
+#if defined(__linux__)
+#include <limits.h>
What's purpose for the header file limits.h?

+#define _MAX_DIR PATH_MAX
Can you use _MAX_PATH in the code to replace _MAX_DIR? _MAX_PATH is
defined here.

Thanks
Liming
+#endif
CHAR8* mGuidToolDefinition = "FmmtConf.ini";
extern EFI_FIRMWARE_VOLUME_HEADER *mFvHeader;
extern UINT32 mFvLength;

//
@@ -1715,15 +1718,10 @@ FmmtImageExtract (
EFI_STATUS Status;
FIRMWARE_DEVICE *FdData;
FV_INFORMATION *FvInFd;
UINT32 Index;
UINT32 FfsFoundFlag;
- FFS_INFORMATION *OutputFileName;
- FILE* NewFdFile;
- FILE* NewFvFile;
- UINT64 NewFvLength;
- VOID* Buffer;
CHAR8 *TemDir;
UINT8 FvNumInFd;
UINT32 Offset;
UINT8 *FdBuffer;
EFI_FFS_FILE_HEADER2 *CurrentFile;
@@ -1738,18 +1736,13 @@ FmmtImageExtract (
int j;
CHAR8 FfsOutputFileName[_MAX_DIR];

FdSize = 0;
Index = 0;
- NewFvLength = 0;
FfsFoundFlag = 0;
FdData = NULL;
FvInFd = NULL;
- OutputFileName = NULL;
- NewFdFile = NULL;
- NewFvFile = NULL;
- Buffer = NULL;
TemDir = NULL;
FvNumInFd = 0;
Offset = 0;
FdBuffer = NULL;
if (sizeof(FfsOutFileOrDirName) > _MAX_DIR) {
--
2.20.1.windows.1





Re: [PATCH v2 0/1] UefiPayloadPkg: Set default PciBaseSize on Ia32

Ma, Maurice <maurice.ma@...>
 

Reviewed-by:
Maurice Ma <maurice.ma@...>

-----Original Message-----
From: Marcello Sylvester Bauer <marcello.bauer@...>
Sent: Tuesday, October 13, 2020 6:34
To: devel@edk2.groups.io
Cc: Marcello Sylvester Bauer <marcello.bauer@...>;
patrick.rudolph@...; Ma, Maurice <maurice.ma@...>; Dong,
Guo <guo.dong@...>; You, Benjamin <benjamin.you@...>
Subject: [PATCH v2 0/1] UefiPayloadPkg: Set default PciBaseSize on Ia32

This commit fix UefiPayloadPkgIa32 build in master.

In commit 8028b2907e20b21cd7d69639a36ac82a77c81dc1 I did forget to set
the default value for PcdPciExpressBaseSize on Ia32 Targets. This patch does
insert it afterwards. It would be great if it could be merged asap.

PS: I added the Ia32 target to our CI to avoid this issue in future. Sorry for the
misfortune.

v2:
* Remove no longer required build-time PcdPciExpressBaseAddress

Branch: https://github.com/9elements/edk2/tree/fix/UefiPayloadPkgIa32_V2
PR: https://github.com/tianocore/edk2/pull/1008

Marcello Sylvester Bauer (1):
UefiPayloadPkg: Set default PciBaseSize on Ia32

UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

--
2.28.0


Re: [PATCH v2 1/1] UefiPayloadPkg: Set default PciBaseSize on Ia32

Ma, Maurice <maurice.ma@...>
 

Reviewed-by:
Maurice Ma <maurice.ma@...>

-----Original Message-----
From: Marcello Sylvester Bauer <marcello.bauer@...>
Sent: Tuesday, October 13, 2020 6:34
To: devel@edk2.groups.io
Cc: Marcello Sylvester Bauer <marcello.bauer@...>;
patrick.rudolph@...; Ma, Maurice <maurice.ma@...>; Dong,
Guo <guo.dong@...>; You, Benjamin <benjamin.you@...>
Subject: [PATCH v2 1/1] UefiPayloadPkg: Set default PciBaseSize on Ia32

* Add needed PcdPciExpressBaseSize default on Ia32 targets analog to X64 in:
8028b2907e20b21cd7d69639a36ac82a77c81dc1
* Remove no longer required build-time PcdPciExpressBaseAddress with regards
to:
3900a63e3a1b9ba9a4105bedead7b986188cec2c

Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@...>
Cc: Patrick Rudolph <patrick.rudolph@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Guo Dong <guo.dong@...>
Cc: Benjamin You <benjamin.you@...>
---
UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
index 12d7ffe81416..d1fcbbb50207 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
@@ -292,8 +292,6 @@ [PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE

gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa,
0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23,
0x31 }



- gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)

-

!if $(SOURCE_DEBUG_ENABLE)

gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2

!endif

@@ -364,6 +362,7 @@ [PcdsDynamicDefault]
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31

gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100

gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0

+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0




#############################################################
###################

#

--
2.28.0


Re: VariablePolicy: Final Changes Thread 1 - TPL Ordering

Bret Barkelew <bret.barkelew@...>
 

I’ll try to take a look at that today. Thanks!

 

- Bret

 

From: Kinney, Michael D
Sent: Tuesday, October 13, 2020 11:22 AM
To: Bret Barkelew; devel@edk2.groups.io; Andrew Fish; Kinney, Michael D
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Bret,

 

The concept of making the platform lock point configurable seems like a good idea from a platform porting perspective.  If we had that feature would that make this better?

 

Have you studied the features DPC provides?  It is functionally similar to having NOTIFY-1 and CALLBACK-1 TPL levels.

 

https://github.com/tianocore/edk2/blob/master/NetworkPkg/DpcDxe/Dpc.c

 

Thanks,

 

Mike

 

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Tuesday, October 13, 2020 10:57 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

The problem with the other solution is that it doesn’t give the platform any flexibility to move which event the lock is attached to. For EDK2 default, we’re making it EndOfDxe, because that matches most platforms’ threat models. For Mu, we use ReadyToBoot.

 

The current “hack” makes that more difficult without multiple patches that we have to carry to remove the “fixed” ordering of the events.

 

That said, I do think I’ll clean that up and submit it, unless Sean reads this and stops me. 😉

 

- Bret

 

From: Michael D Kinney via groups.io
Sent: Tuesday, October 13, 2020 8:05 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Bret,

 

If the other solution more directly expresses the dependency and guarantees ordering, then it may not be considered a hack.

 

I prefer solutions that minimize the chances of failures and the need for platform developers to do extra work to integrate a feature or do extra work when they add an unrelated feature.

 

I see Laszlo’s proposal to add a new tag GUID protocol to detect and enforce ordering for this specific use case.

 

The network stack added a DPC (Deferred Procedure Call) Protocol to help with ordering issues and remove the introduction of extra TPL levels in an earlier version of the network stack.  I am wondering of there is a more generic solution to this specific problem through the use of DPC.

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Monday, October 12, 2020 3:14 PM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Like I said, I’m also happy to go with the lesser solution of replacing the hack that was already in the code. The last person didn’t care to solve this problem, and I’m good to not solve it, too. I mean, I think it’s turtles all the way down no matter what.

 

It was actually the ASSERT in the code that highlighted this problem to being with, so I would say that it’s doing its job. It’s incumbent upon the code author to determine what resource they’re trying to access and whether they’ve accessed it successfully, and I agree that it seems like an appropriate use of ASSERTs so long as it’s backed up with some OTHER appropriate action (even if that action is ignoring it).

 

- Bret

 

From: Kinney, Michael D
Sent: Monday, October 12, 2020 9:44 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Bret,

 

How to platform creators know for the complete set of drivers if there is anything then need to worry about and why and what they need to address the concern?  This is about order that events are signaled for a given event trigger.  When a platform adds more driver that may use the same event triggers, how do they know if there is a potential for a race condition or not?  If event notification functions are design to be independent of signaling order, then there is no issue.  As soon as there are requirements for event notification functions to be executed in a specific order at a specific event trigger, we have to make sure the platform creator knows and preferably, the FW can tell them if they got it wrong.

 

Can your data/device manipulators and data/device protectors use case generate an ASSERT() if they are signaled in the wrong order?

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Wednesday, October 7, 2020 9:39 AM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Agreed with your concern, Mike. This mechanism (and we can document it as such) should NOT be used to accomplish an explicit ordering (a la the “apriori list”). It’s just to provide a little separation for two patterns that we’ve seen time and again in our code: data/device manipulators and data/device protectors. It does not eliminate the necessity for platform creators to understand things like driver ordering if they have one driver that requires a protocol be installed or a bus connected.

 

- Bret

 

From: Kinney, Michael D
Sent: Wednesday, October 7, 2020 9:21 AM
To: Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Cc: Bret Barkelew
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Andrew,

 

I agree DXE drivers could use a PCD to make it configurable and prevent collisions with UEFI defined TPL levels.

 

Bret’s suggestion of adding a DXE scoped services to create events using non-UEFI defined TPL levels could be used with these TPL levels from PCDs.  Would also allow DXE drivers to use TPL levels associated with the firmware interrupts in the range 17..30.  Perhaps extensions to the DXE Services Table?

 

Still does not address my concern that many DXE drivers using these extra TPL levels may run into race conditions if more than one DXE driver selects the same TPL level.  Platform integrators will need to understand the relative priorities of all DXE drivers that use extra TPL levels so they can assign values that both avoid collisions with future UEFI specs and prevent race conditions among DXE drivers.

 

Mike

 

From: Andrew Fish <afish@...>
Sent: Tuesday, October 6, 2020 7:18 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@...>
Cc: bret.barkelew@...
Subject: Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

Mike,

 

When I’ve done things like this in the past I think of making them configurable like via a PCD. 

 

In terms of the #defines I think it makes more sense to just do math on the spec defined values. So TPL_CALLBACK + 1 or TPL_CALLBACK - 1 etc.  I’ve got an lldb type formatter for TPL and it prints out <UEFI Spec TPL> [+ <extra>] as I think this is the clearest way to do it. 

 

Thanks,

 

Andrew Fish

 

On Oct 6, 2020, at 6:54 PM, Michael D Kinney <michael.d.kinney@...> wrote:

 

Bret,

 

It is likely best to go with the first approach.  The discussion on TPL levels can continue and you could adopt it in the future if a general solution is identified.

 

TPL 17..30 are reserved by the UEFI Spec for firmware interrupts.  So TPL_NOTIFY_HIGH as defined would not be allowed.

 

I agree that the use of TPL values other than those defined by the UEFI Spec can potentially be used by DXE.  However, that DXE usage must be flexible enough to handle a future extension to the UEFI Spec for new TPL levels without a collision.

 

Instead of defining specific TPL values, you could add a DXE scoped service to allocate the use of a new TPL level that is not being used by UEFI or other DXE drivers.  I will point out that these approaches (defining new TPL levels or allocating unused TPL levels) just moves the same problem.  You can solve it for the first driver that needs something special.  As soon as there is more than one driver that need something special at the same TPL level, the potential for a race condition for ordering will show up again.  So I do not consider adding TPL levels to be a good general solution to this problem.

 

Best regards,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Tuesday, October 6, 2020 5:24 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

As many will be aware, I’m in the final stages of having Variable Policy ready for commit. However, after moving the “Lock” event back to EndOfDxe, this exposed a race condition in variable services.

 

A quick synopsis of the problem:

  • Previously, MorLock abused a privileged position by being tightly coupled to Variable Services, and its lock callback was called directly so that it could be strongly ordered with the internal property lock that disables future RequestToLock calls.
  • VariablePolicy attempted to decouple this (without realizing it was a problem) and go through formalized interfaces that could ultimately be broken out of Variable Services altogether.
  • However, doing so triggered the race condition, causing an ASSERT when MorLock attempted to lock its variables.
  • I current have a reimplementation of the strong ordering workaround that can be previewed at the link below. I have tested that it works the same as the OLD system.

 

However, replacing one bad design with another is not what this community is about (when we can help it), so we’d like to take a moment to revisit a conversation that has come up before: expanding the number of supported TPL levels.

 

Now, I know that the current TPL levels are defined in the UEFI spec and we don’t want to have to change those, but there’s no reason that we can come up with not to add some more granularity in the PI spec, dedicated to platform and implementation ordering (the UEFI spec events will have to remain on UEFI spec TPLs). Basically there would be a set of DXE Services that allow WaitForEvent, CheckEvent, Event registration at TPLs other than notify/callback.  The UEFI system table versions of the functions would still have this restriction but code built with the platform could use the DXE Services. Right now, any attempt to use a non-UEFI TPL will ASSERT, so we can keep that in place on the SystemTable interface, but allow the platform to go around it with DXE Services. Similar functionality would have to be provided by the Mmst, but that’s already platform-specific and can probably allow it in all cases.

 

We’re suggesting something like the below:

 

//

// Task priority level

//

#define TPL_APPLICATION       4

#define TPL_CALLBACK_LOW      7

#define TPL_CALLBACK          8

#define TPL_CALLBACK_HIGH     9

#define TPL_NOTIFY_LOW        15

#define TPL_NOTIFY            16

#define TPL_NOTIFY_HIGH       17

#define TPL_HIGH_LEVEL        31

 

There’s already a long-in-the-tooth bug tracking a similar issue:

 

This proposal is simpler than what’s in that bug, and would greatly simplify some of our event ordering (and code).

 

Thoughts?

 

If this conversation takes too long, I will publish a set of patches that just go with the lesser solution posted above, but I’d much rather work the problem this way.

 

- Bret

 

 

 

 

 


Re: VariablePolicy: Final Changes Thread 1 - TPL Ordering

Michael D Kinney
 

Hi Bret,

 

The concept of making the platform lock point configurable seems like a good idea from a platform porting perspective.  If we had that feature would that make this better?

 

Have you studied the features DPC provides?  It is functionally similar to having NOTIFY-1 and CALLBACK-1 TPL levels.

 

https://github.com/tianocore/edk2/blob/master/NetworkPkg/DpcDxe/Dpc.c

 

Thanks,

 

Mike

 

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Tuesday, October 13, 2020 10:57 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

The problem with the other solution is that it doesn’t give the platform any flexibility to move which event the lock is attached to. For EDK2 default, we’re making it EndOfDxe, because that matches most platforms’ threat models. For Mu, we use ReadyToBoot.

 

The current “hack” makes that more difficult without multiple patches that we have to carry to remove the “fixed” ordering of the events.

 

That said, I do think I’ll clean that up and submit it, unless Sean reads this and stops me. 😉

 

- Bret

 

From: Michael D Kinney via groups.io
Sent: Tuesday, October 13, 2020 8:05 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Bret,

 

If the other solution more directly expresses the dependency and guarantees ordering, then it may not be considered a hack.

 

I prefer solutions that minimize the chances of failures and the need for platform developers to do extra work to integrate a feature or do extra work when they add an unrelated feature.

 

I see Laszlo’s proposal to add a new tag GUID protocol to detect and enforce ordering for this specific use case.

 

The network stack added a DPC (Deferred Procedure Call) Protocol to help with ordering issues and remove the introduction of extra TPL levels in an earlier version of the network stack.  I am wondering of there is a more generic solution to this specific problem through the use of DPC.

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Monday, October 12, 2020 3:14 PM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Like I said, I’m also happy to go with the lesser solution of replacing the hack that was already in the code. The last person didn’t care to solve this problem, and I’m good to not solve it, too. I mean, I think it’s turtles all the way down no matter what.

 

It was actually the ASSERT in the code that highlighted this problem to being with, so I would say that it’s doing its job. It’s incumbent upon the code author to determine what resource they’re trying to access and whether they’ve accessed it successfully, and I agree that it seems like an appropriate use of ASSERTs so long as it’s backed up with some OTHER appropriate action (even if that action is ignoring it).

 

- Bret

 

From: Kinney, Michael D
Sent: Monday, October 12, 2020 9:44 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Bret,

 

How to platform creators know for the complete set of drivers if there is anything then need to worry about and why and what they need to address the concern?  This is about order that events are signaled for a given event trigger.  When a platform adds more driver that may use the same event triggers, how do they know if there is a potential for a race condition or not?  If event notification functions are design to be independent of signaling order, then there is no issue.  As soon as there are requirements for event notification functions to be executed in a specific order at a specific event trigger, we have to make sure the platform creator knows and preferably, the FW can tell them if they got it wrong.

 

Can your data/device manipulators and data/device protectors use case generate an ASSERT() if they are signaled in the wrong order?

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Wednesday, October 7, 2020 9:39 AM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Agreed with your concern, Mike. This mechanism (and we can document it as such) should NOT be used to accomplish an explicit ordering (a la the “apriori list”). It’s just to provide a little separation for two patterns that we’ve seen time and again in our code: data/device manipulators and data/device protectors. It does not eliminate the necessity for platform creators to understand things like driver ordering if they have one driver that requires a protocol be installed or a bus connected.

 

- Bret

 

From: Kinney, Michael D
Sent: Wednesday, October 7, 2020 9:21 AM
To: Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Cc: Bret Barkelew
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Andrew,

 

I agree DXE drivers could use a PCD to make it configurable and prevent collisions with UEFI defined TPL levels.

 

Bret’s suggestion of adding a DXE scoped services to create events using non-UEFI defined TPL levels could be used with these TPL levels from PCDs.  Would also allow DXE drivers to use TPL levels associated with the firmware interrupts in the range 17..30.  Perhaps extensions to the DXE Services Table?

 

Still does not address my concern that many DXE drivers using these extra TPL levels may run into race conditions if more than one DXE driver selects the same TPL level.  Platform integrators will need to understand the relative priorities of all DXE drivers that use extra TPL levels so they can assign values that both avoid collisions with future UEFI specs and prevent race conditions among DXE drivers.

 

Mike

 

From: Andrew Fish <afish@...>
Sent: Tuesday, October 6, 2020 7:18 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@...>
Cc: bret.barkelew@...
Subject: Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

Mike,

 

When I’ve done things like this in the past I think of making them configurable like via a PCD. 

 

In terms of the #defines I think it makes more sense to just do math on the spec defined values. So TPL_CALLBACK + 1 or TPL_CALLBACK - 1 etc.  I’ve got an lldb type formatter for TPL and it prints out <UEFI Spec TPL> [+ <extra>] as I think this is the clearest way to do it. 

 

Thanks,

 

Andrew Fish

 

On Oct 6, 2020, at 6:54 PM, Michael D Kinney <michael.d.kinney@...> wrote:

 

Bret,

 

It is likely best to go with the first approach.  The discussion on TPL levels can continue and you could adopt it in the future if a general solution is identified.

 

TPL 17..30 are reserved by the UEFI Spec for firmware interrupts.  So TPL_NOTIFY_HIGH as defined would not be allowed.

 

I agree that the use of TPL values other than those defined by the UEFI Spec can potentially be used by DXE.  However, that DXE usage must be flexible enough to handle a future extension to the UEFI Spec for new TPL levels without a collision.

 

Instead of defining specific TPL values, you could add a DXE scoped service to allocate the use of a new TPL level that is not being used by UEFI or other DXE drivers.  I will point out that these approaches (defining new TPL levels or allocating unused TPL levels) just moves the same problem.  You can solve it for the first driver that needs something special.  As soon as there is more than one driver that need something special at the same TPL level, the potential for a race condition for ordering will show up again.  So I do not consider adding TPL levels to be a good general solution to this problem.

 

Best regards,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Tuesday, October 6, 2020 5:24 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

As many will be aware, I’m in the final stages of having Variable Policy ready for commit. However, after moving the “Lock” event back to EndOfDxe, this exposed a race condition in variable services.

 

A quick synopsis of the problem:

  • Previously, MorLock abused a privileged position by being tightly coupled to Variable Services, and its lock callback was called directly so that it could be strongly ordered with the internal property lock that disables future RequestToLock calls.
  • VariablePolicy attempted to decouple this (without realizing it was a problem) and go through formalized interfaces that could ultimately be broken out of Variable Services altogether.
  • However, doing so triggered the race condition, causing an ASSERT when MorLock attempted to lock its variables.
  • I current have a reimplementation of the strong ordering workaround that can be previewed at the link below. I have tested that it works the same as the OLD system.

 

However, replacing one bad design with another is not what this community is about (when we can help it), so we’d like to take a moment to revisit a conversation that has come up before: expanding the number of supported TPL levels.

 

Now, I know that the current TPL levels are defined in the UEFI spec and we don’t want to have to change those, but there’s no reason that we can come up with not to add some more granularity in the PI spec, dedicated to platform and implementation ordering (the UEFI spec events will have to remain on UEFI spec TPLs). Basically there would be a set of DXE Services that allow WaitForEvent, CheckEvent, Event registration at TPLs other than notify/callback.  The UEFI system table versions of the functions would still have this restriction but code built with the platform could use the DXE Services. Right now, any attempt to use a non-UEFI TPL will ASSERT, so we can keep that in place on the SystemTable interface, but allow the platform to go around it with DXE Services. Similar functionality would have to be provided by the Mmst, but that’s already platform-specific and can probably allow it in all cases.

 

We’re suggesting something like the below:

 

//

// Task priority level

//

#define TPL_APPLICATION       4

#define TPL_CALLBACK_LOW      7

#define TPL_CALLBACK          8

#define TPL_CALLBACK_HIGH     9

#define TPL_NOTIFY_LOW        15

#define TPL_NOTIFY            16

#define TPL_NOTIFY_HIGH       17

#define TPL_HIGH_LEVEL        31

 

There’s already a long-in-the-tooth bug tracking a similar issue:

 

This proposal is simpler than what’s in that bug, and would greatly simplify some of our event ordering (and code).

 

Thoughts?

 

If this conversation takes too long, I will publish a set of patches that just go with the lesser solution posted above, but I’d much rather work the problem this way.

 

- Bret

 

 

 

 


Re: VariablePolicy: Final Changes Thread 1 - TPL Ordering

Bret Barkelew <bret.barkelew@...>
 

The problem with the other solution is that it doesn’t give the platform any flexibility to move which event the lock is attached to. For EDK2 default, we’re making it EndOfDxe, because that matches most platforms’ threat models. For Mu, we use ReadyToBoot.

 

The current “hack” makes that more difficult without multiple patches that we have to carry to remove the “fixed” ordering of the events.

 

That said, I do think I’ll clean that up and submit it, unless Sean reads this and stops me. 😉

 

- Bret

 

From: Michael D Kinney via groups.io
Sent: Tuesday, October 13, 2020 8:05 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Bret,

 

If the other solution more directly expresses the dependency and guarantees ordering, then it may not be considered a hack.

 

I prefer solutions that minimize the chances of failures and the need for platform developers to do extra work to integrate a feature or do extra work when they add an unrelated feature.

 

I see Laszlo’s proposal to add a new tag GUID protocol to detect and enforce ordering for this specific use case.

 

The network stack added a DPC (Deferred Procedure Call) Protocol to help with ordering issues and remove the introduction of extra TPL levels in an earlier version of the network stack.  I am wondering of there is a more generic solution to this specific problem through the use of DPC.

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Monday, October 12, 2020 3:14 PM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Like I said, I’m also happy to go with the lesser solution of replacing the hack that was already in the code. The last person didn’t care to solve this problem, and I’m good to not solve it, too. I mean, I think it’s turtles all the way down no matter what.

 

It was actually the ASSERT in the code that highlighted this problem to being with, so I would say that it’s doing its job. It’s incumbent upon the code author to determine what resource they’re trying to access and whether they’ve accessed it successfully, and I agree that it seems like an appropriate use of ASSERTs so long as it’s backed up with some OTHER appropriate action (even if that action is ignoring it).

 

- Bret

 

From: Kinney, Michael D
Sent: Monday, October 12, 2020 9:44 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Bret,

 

How to platform creators know for the complete set of drivers if there is anything then need to worry about and why and what they need to address the concern?  This is about order that events are signaled for a given event trigger.  When a platform adds more driver that may use the same event triggers, how do they know if there is a potential for a race condition or not?  If event notification functions are design to be independent of signaling order, then there is no issue.  As soon as there are requirements for event notification functions to be executed in a specific order at a specific event trigger, we have to make sure the platform creator knows and preferably, the FW can tell them if they got it wrong.

 

Can your data/device manipulators and data/device protectors use case generate an ASSERT() if they are signaled in the wrong order?

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Wednesday, October 7, 2020 9:39 AM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Agreed with your concern, Mike. This mechanism (and we can document it as such) should NOT be used to accomplish an explicit ordering (a la the “apriori list”). It’s just to provide a little separation for two patterns that we’ve seen time and again in our code: data/device manipulators and data/device protectors. It does not eliminate the necessity for platform creators to understand things like driver ordering if they have one driver that requires a protocol be installed or a bus connected.

 

- Bret

 

From: Kinney, Michael D
Sent: Wednesday, October 7, 2020 9:21 AM
To: Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Cc: Bret Barkelew
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Andrew,

 

I agree DXE drivers could use a PCD to make it configurable and prevent collisions with UEFI defined TPL levels.

 

Bret’s suggestion of adding a DXE scoped services to create events using non-UEFI defined TPL levels could be used with these TPL levels from PCDs.  Would also allow DXE drivers to use TPL levels associated with the firmware interrupts in the range 17..30.  Perhaps extensions to the DXE Services Table?

 

Still does not address my concern that many DXE drivers using these extra TPL levels may run into race conditions if more than one DXE driver selects the same TPL level.  Platform integrators will need to understand the relative priorities of all DXE drivers that use extra TPL levels so they can assign values that both avoid collisions with future UEFI specs and prevent race conditions among DXE drivers.

 

Mike

 

From: Andrew Fish <afish@...>
Sent: Tuesday, October 6, 2020 7:18 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@...>
Cc: bret.barkelew@...
Subject: Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

Mike,

 

When I’ve done things like this in the past I think of making them configurable like via a PCD. 

 

In terms of the #defines I think it makes more sense to just do math on the spec defined values. So TPL_CALLBACK + 1 or TPL_CALLBACK - 1 etc.  I’ve got an lldb type formatter for TPL and it prints out <UEFI Spec TPL> [+ <extra>] as I think this is the clearest way to do it. 

 

Thanks,

 

Andrew Fish

 

On Oct 6, 2020, at 6:54 PM, Michael D Kinney <michael.d.kinney@...> wrote:

 

Bret,

 

It is likely best to go with the first approach.  The discussion on TPL levels can continue and you could adopt it in the future if a general solution is identified.

 

TPL 17..30 are reserved by the UEFI Spec for firmware interrupts.  So TPL_NOTIFY_HIGH as defined would not be allowed.

 

I agree that the use of TPL values other than those defined by the UEFI Spec can potentially be used by DXE.  However, that DXE usage must be flexible enough to handle a future extension to the UEFI Spec for new TPL levels without a collision.

 

Instead of defining specific TPL values, you could add a DXE scoped service to allocate the use of a new TPL level that is not being used by UEFI or other DXE drivers.  I will point out that these approaches (defining new TPL levels or allocating unused TPL levels) just moves the same problem.  You can solve it for the first driver that needs something special.  As soon as there is more than one driver that need something special at the same TPL level, the potential for a race condition for ordering will show up again.  So I do not consider adding TPL levels to be a good general solution to this problem.

 

Best regards,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Tuesday, October 6, 2020 5:24 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

As many will be aware, I’m in the final stages of having Variable Policy ready for commit. However, after moving the “Lock” event back to EndOfDxe, this exposed a race condition in variable services.

 

A quick synopsis of the problem:

  • Previously, MorLock abused a privileged position by being tightly coupled to Variable Services, and its lock callback was called directly so that it could be strongly ordered with the internal property lock that disables future RequestToLock calls.
  • VariablePolicy attempted to decouple this (without realizing it was a problem) and go through formalized interfaces that could ultimately be broken out of Variable Services altogether.
  • However, doing so triggered the race condition, causing an ASSERT when MorLock attempted to lock its variables.
  • I current have a reimplementation of the strong ordering workaround that can be previewed at the link below. I have tested that it works the same as the OLD system.

 

However, replacing one bad design with another is not what this community is about (when we can help it), so we’d like to take a moment to revisit a conversation that has come up before: expanding the number of supported TPL levels.

 

Now, I know that the current TPL levels are defined in the UEFI spec and we don’t want to have to change those, but there’s no reason that we can come up with not to add some more granularity in the PI spec, dedicated to platform and implementation ordering (the UEFI spec events will have to remain on UEFI spec TPLs). Basically there would be a set of DXE Services that allow WaitForEvent, CheckEvent, Event registration at TPLs other than notify/callback.  The UEFI system table versions of the functions would still have this restriction but code built with the platform could use the DXE Services. Right now, any attempt to use a non-UEFI TPL will ASSERT, so we can keep that in place on the SystemTable interface, but allow the platform to go around it with DXE Services. Similar functionality would have to be provided by the Mmst, but that’s already platform-specific and can probably allow it in all cases.

 

We’re suggesting something like the below:

 

//

// Task priority level

//

#define TPL_APPLICATION       4

#define TPL_CALLBACK_LOW      7

#define TPL_CALLBACK          8

#define TPL_CALLBACK_HIGH     9

#define TPL_NOTIFY_LOW        15

#define TPL_NOTIFY            16

#define TPL_NOTIFY_HIGH       17

#define TPL_HIGH_LEVEL        31

 

There’s already a long-in-the-tooth bug tracking a similar issue:

 

This proposal is simpler than what’s in that bug, and would greatly simplify some of our event ordering (and code).

 

Thoughts?

 

If this conversation takes too long, I will publish a set of patches that just go with the lesser solution posted above, but I’d much rather work the problem this way.

 

- Bret

 

 

 

 


[PATCH v2 1/1] Silicon/Qemu/Sbsa: Add SBSA-wdt entry to GTDT

Shashi Mallela
 

SBSA generic watchdog timer structure entry has been added
to GTDT table as per BSAv0.9.
This enables acpi detection of wdt in qemu sbsa platform

Signed-off-by: Shashi Mallela <shashi.mallela@...>
---
Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc | 53 +++++++++++++++++---
1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
index d16778e01a5c..2312fd74e26d 100644
--- a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
+++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
@@ -24,27 +24,55 @@
#define SYSTEM_TIMER_BASE_ADDRESS 0xFFFFFFFFFFFFFFFF
#endif

-#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
#define GTDT_TIMER_LEVEL_TRIGGERED 0
-#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
#define GTDT_TIMER_ACTIVE_HIGH 0

#define GTDT_GTIMER_FLAGS (GTDT_TIMER_ACTIVE_LOW | GTDT_TIMER_LEVEL_TRIGGERED)

+#define SBSA_PLATFORM_WATCHDOG_COUNT 1
+#define SBSA_PLATFORM_TIMER_COUNT (SBSA_PLATFORM_WATCHDOG_COUNT)
+
+#define SBSAQEMU_WDT_REFRESH_FRAME_BASE 0x50010000
+#define SBSAQEMU_WDT_CONTROL_FRAME_BASE 0x50011000
+#define SBSAQEMU_WDT_IRQ 44
+
+#define GTDT_WDTIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_WDTIMER_LEVEL_TRIGGERED 0
+#define GTDT_WDTIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_WDTIMER_ACTIVE_HIGH 0
+
+#define GTDT_WDTIMER_FLAGS (GTDT_WDTIMER_ACTIVE_HIGH | GTDT_WDTIMER_LEVEL_TRIGGERED)
+
+#define EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT( \
+ RefreshFramePhysicalAddress, ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, WatchdogTimerFlags) \
+ { \
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG, \
+ sizeof(EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE), \
+ EFI_ACPI_RESERVED_WORD, \
+ RefreshFramePhysicalAddress, \
+ ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, \
+ WatchdogTimerFlags \
+ }
+
#pragma pack (1)

typedef struct {
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
- } GENERIC_TIMER_DESCRIPTION_TABLE;
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE Gwdt;
+ } GENERIC_TIMER_DESCRIPTION_TABLES;

#pragma pack ()

GENERIC_TIMER_DESCRIPTION_TABLE Gtdt = {
{
SBSAQEMU_ACPI_HEADER(
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
GENERIC_TIMER_DESCRIPTION_TABLE,
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
),
SYSTEM_TIMER_BASE_ADDRESS, // UINT64 PhysicalAddress
0, // UINT32 Reserved
@@ -57,9 +85,18 @@
FixedPcdGet32 (PcdArmArchTimerHypIntrNum), // UINT32 NonSecurePL2TimerGSIV
GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL2TimerFlags
0xFFFFFFFFFFFFFFFF, // UINT64 CntReadBasePhysicalAddress
- 0, // UINT32 PlatformTimerCount
- 0
+ SBSA_PLATFORM_TIMER_COUNT, // UINT32 PlatformTimerCount
+ sizeof(EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE),
+ // UINT32 PlatformTimerOffset
+ 0, // UINT32 VirtualPL2TimerGSIV
+ 0 // UINT32 VirtualPL2TimerFlags
},
+ EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
+ SBSAQEMU_WDT_REFRESH_FRAME_BASE,
+ SBSAQEMU_WDT_CONTROL_FRAME_BASE,
+ SBSAQEMU_WDT_IRQ,
+ GTDT_WDTIMER_FLAGS
+ )
};

// Reference the table being generated to prevent the optimizer from removing the
--
2.18.4


[PATCH v2 0/1] Add SBSA-wdt entry to GTDT

Shashi Mallela
 

To enable detection of qemu SBSA generic watchdog timer device,this
patch has been added to create sbsa-wdt entry into the GTDT table
which helps firmware report the presence of SBSA-wdt to the OS.

Changes in v2:
- fixed patch indentation

Shashi Mallela (1):
Silicon/Qemu/Sbsa: Add SBSA-wdt entry to GTDT

Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc | 53 +++++++++++++++++---
1 file changed, 45 insertions(+), 8 deletions(-)

--
2.18.4


Re: VariablePolicy: Final Changes Thread 1 - TPL Ordering

Michael D Kinney
 

Hi Bret,

 

If the other solution more directly expresses the dependency and guarantees ordering, then it may not be considered a hack.

 

I prefer solutions that minimize the chances of failures and the need for platform developers to do extra work to integrate a feature or do extra work when they add an unrelated feature.

 

I see Laszlo’s proposal to add a new tag GUID protocol to detect and enforce ordering for this specific use case.

 

The network stack added a DPC (Deferred Procedure Call) Protocol to help with ordering issues and remove the introduction of extra TPL levels in an earlier version of the network stack.  I am wondering of there is a more generic solution to this specific problem through the use of DPC.

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Monday, October 12, 2020 3:14 PM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Like I said, I’m also happy to go with the lesser solution of replacing the hack that was already in the code. The last person didn’t care to solve this problem, and I’m good to not solve it, too. I mean, I think it’s turtles all the way down no matter what.

 

It was actually the ASSERT in the code that highlighted this problem to being with, so I would say that it’s doing its job. It’s incumbent upon the code author to determine what resource they’re trying to access and whether they’ve accessed it successfully, and I agree that it seems like an appropriate use of ASSERTs so long as it’s backed up with some OTHER appropriate action (even if that action is ignoring it).

 

- Bret

 

From: Kinney, Michael D
Sent: Monday, October 12, 2020 9:44 AM
To: Bret Barkelew; Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Bret,

 

How to platform creators know for the complete set of drivers if there is anything then need to worry about and why and what they need to address the concern?  This is about order that events are signaled for a given event trigger.  When a platform adds more driver that may use the same event triggers, how do they know if there is a potential for a race condition or not?  If event notification functions are design to be independent of signaling order, then there is no issue.  As soon as there are requirements for event notification functions to be executed in a specific order at a specific event trigger, we have to make sure the platform creator knows and preferably, the FW can tell them if they got it wrong.

 

Can your data/device manipulators and data/device protectors use case generate an ASSERT() if they are signaled in the wrong order?

 

Mike

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Wednesday, October 7, 2020 9:39 AM
To: Kinney, Michael D <michael.d.kinney@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Subject: RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Agreed with your concern, Mike. This mechanism (and we can document it as such) should NOT be used to accomplish an explicit ordering (a la the “apriori list”). It’s just to provide a little separation for two patterns that we’ve seen time and again in our code: data/device manipulators and data/device protectors. It does not eliminate the necessity for platform creators to understand things like driver ordering if they have one driver that requires a protocol be installed or a bus connected.

 

- Bret

 

From: Kinney, Michael D
Sent: Wednesday, October 7, 2020 9:21 AM
To: Andrew Fish; edk2-devel-groups-io; Kinney, Michael D
Cc: Bret Barkelew
Subject: [EXTERNAL] RE: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

Hi Andrew,

 

I agree DXE drivers could use a PCD to make it configurable and prevent collisions with UEFI defined TPL levels.

 

Bret’s suggestion of adding a DXE scoped services to create events using non-UEFI defined TPL levels could be used with these TPL levels from PCDs.  Would also allow DXE drivers to use TPL levels associated with the firmware interrupts in the range 17..30.  Perhaps extensions to the DXE Services Table?

 

Still does not address my concern that many DXE drivers using these extra TPL levels may run into race conditions if more than one DXE driver selects the same TPL level.  Platform integrators will need to understand the relative priorities of all DXE drivers that use extra TPL levels so they can assign values that both avoid collisions with future UEFI specs and prevent race conditions among DXE drivers.

 

Mike

 

From: Andrew Fish <afish@...>
Sent: Tuesday, October 6, 2020 7:18 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@...>
Cc: bret.barkelew@...
Subject: Re: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

Mike,

 

When I’ve done things like this in the past I think of making them configurable like via a PCD. 

 

In terms of the #defines I think it makes more sense to just do math on the spec defined values. So TPL_CALLBACK + 1 or TPL_CALLBACK - 1 etc.  I’ve got an lldb type formatter for TPL and it prints out <UEFI Spec TPL> [+ <extra>] as I think this is the clearest way to do it. 

 

Thanks,

 

Andrew Fish

 

On Oct 6, 2020, at 6:54 PM, Michael D Kinney <michael.d.kinney@...> wrote:

 

Bret,

 

It is likely best to go with the first approach.  The discussion on TPL levels can continue and you could adopt it in the future if a general solution is identified.

 

TPL 17..30 are reserved by the UEFI Spec for firmware interrupts.  So TPL_NOTIFY_HIGH as defined would not be allowed.

 

I agree that the use of TPL values other than those defined by the UEFI Spec can potentially be used by DXE.  However, that DXE usage must be flexible enough to handle a future extension to the UEFI Spec for new TPL levels without a collision.

 

Instead of defining specific TPL values, you could add a DXE scoped service to allocate the use of a new TPL level that is not being used by UEFI or other DXE drivers.  I will point out that these approaches (defining new TPL levels or allocating unused TPL levels) just moves the same problem.  You can solve it for the first driver that needs something special.  As soon as there is more than one driver that need something special at the same TPL level, the potential for a race condition for ordering will show up again.  So I do not consider adding TPL levels to be a good general solution to this problem.

 

Best regards,

 

Mike

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Tuesday, October 6, 2020 5:24 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] VariablePolicy: Final Changes Thread 1 - TPL Ordering

 

As many will be aware, I’m in the final stages of having Variable Policy ready for commit. However, after moving the “Lock” event back to EndOfDxe, this exposed a race condition in variable services.

 

A quick synopsis of the problem:

  • Previously, MorLock abused a privileged position by being tightly coupled to Variable Services, and its lock callback was called directly so that it could be strongly ordered with the internal property lock that disables future RequestToLock calls.
  • VariablePolicy attempted to decouple this (without realizing it was a problem) and go through formalized interfaces that could ultimately be broken out of Variable Services altogether.
  • However, doing so triggered the race condition, causing an ASSERT when MorLock attempted to lock its variables.
  • I current have a reimplementation of the strong ordering workaround that can be previewed at the link below. I have tested that it works the same as the OLD system.

 

However, replacing one bad design with another is not what this community is about (when we can help it), so we’d like to take a moment to revisit a conversation that has come up before: expanding the number of supported TPL levels.

 

Now, I know that the current TPL levels are defined in the UEFI spec and we don’t want to have to change those, but there’s no reason that we can come up with not to add some more granularity in the PI spec, dedicated to platform and implementation ordering (the UEFI spec events will have to remain on UEFI spec TPLs). Basically there would be a set of DXE Services that allow WaitForEvent, CheckEvent, Event registration at TPLs other than notify/callback.  The UEFI system table versions of the functions would still have this restriction but code built with the platform could use the DXE Services. Right now, any attempt to use a non-UEFI TPL will ASSERT, so we can keep that in place on the SystemTable interface, but allow the platform to go around it with DXE Services. Similar functionality would have to be provided by the Mmst, but that’s already platform-specific and can probably allow it in all cases.

 

We’re suggesting something like the below:

 

//

// Task priority level

//

#define TPL_APPLICATION       4

#define TPL_CALLBACK_LOW      7

#define TPL_CALLBACK          8

#define TPL_CALLBACK_HIGH     9

#define TPL_NOTIFY_LOW        15

#define TPL_NOTIFY            16

#define TPL_NOTIFY_HIGH       17

#define TPL_HIGH_LEVEL        31

 

There’s already a long-in-the-tooth bug tracking a similar issue:

 

This proposal is simpler than what’s in that bug, and would greatly simplify some of our event ordering (and code).

 

Thoughts?

 

If this conversation takes too long, I will publish a set of patches that just go with the lesser solution posted above, but I’d much rather work the problem this way.

 

- Bret

 

 

 


[PATCH v2 1/1] UefiPayloadPkg: Set default PciBaseSize on Ia32

Marcello Sylvester Bauer <marcello.bauer@...>
 

* Add needed PcdPciExpressBaseSize default on Ia32 targets analog to X64 in:
8028b2907e20b21cd7d69639a36ac82a77c81dc1
* Remove no longer required build-time PcdPciExpressBaseAddress with regard=
s to:
3900a63e3a1b9ba9a4105bedead7b986188cec2c

Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@...>
Cc: Patrick Rudolph <patrick.rudolph@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Guo Dong <guo.dong@...>
Cc: Benjamin You <benjamin.you@...>
---
UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPay=
loadPkgIa32.dsc
index 12d7ffe81416..d1fcbbb50207 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
@@ -292,8 +292,6 @@ [PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c=
, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0=
x31 }=0D
=0D
- gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)=0D
-=0D
!if $(SOURCE_DEBUG_ENABLE)=0D
gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2=0D
!endif=0D
@@ -364,6 +362,7 @@ [PcdsDynamicDefault]
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100=0D
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0=0D
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0=0D
=0D
##########################################################################=
######=0D
#=0D
--=20
2.28.0