Date   

Re: [edk2 PATCH] MdeModulePkg: Fix typo in error message

Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
A
Sent: Friday, July 30, 2021 4:51 PM
To: devel@edk2.groups.io; shpark.zilla@gmail.com
Cc: Chen, Chen A <chen.a.chen@intel.com>; Seonghyun Park
<shpark1@protonmail.com>
Subject: Re: [edk2-devel] [edk2 PATCH] MdeModulePkg: Fix typo in error
message

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Seonghyun Park
Sent: Thursday, July 29, 2021 9:01 AM
To: devel@edk2.groups.io
Cc: Chen, Chen A <chen.a.chen@intel.com>; Seonghyun Park
<shpark1@protonmail.com>
Subject: [edk2-devel] [edk2 PATCH] MdeModulePkg: Fix typo in error
message

Fix typo in error message in CapsuleApp.

Signed-off-by: Seonghyun Park <shpark1@protonmail.com>
---
MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c
b/MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c
index dba50b3202..712cf2e1f7 100644
--- a/MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c
+++ b/MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c
@@ -509,7 +509,7 @@ GetUpdateFileSystem (
DevicePath = DuplicateDevicePath (MappedDevicePath);

Status = GetEfiSysPartitionFromDevPath (DevicePath, &FullPath,
Fs);

if (EFI_ERROR (Status)) {

- Print (L"Error: Cannot get EFI system partiion from '%s' - %r\n", Map,
Status);

+ Print (L"Error: Cannot get EFI system partition from '%s' -
+ %r\n", Map,
Status);

Thanks for the patch.

I will change the commit title a little bit when merging:
MdeModulePkg/CapsuleApp: Fix typo in error message

The change itself is good to me:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Pushed via:
PR - https://github.com/tianocore/edk2/pull/1852
Commit - https://github.com/tianocore/edk2/commit/3445058aea4f64a994e4ec040135258a135d36ce

Best Regards,
Hao Wu



Best Regards,
Hao Wu



return EFI_NOT_FOUND;

}

Print (L"Warning: Cannot find Boot Option on '%s'!\n", Map);

--
2.32.0



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78376):
https://edk2.groups.io/g/devel/message/78376
Mute This Topic: https://groups.io/mt/84528989/1768737
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
-=-=-=-=-=-=




Re: [PATCH EDK2 v2 1/1] MdeModulePkg/UefiSortLib:Add UefiSortLib unit test

Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
A
Sent: Thursday, July 29, 2021 4:26 PM
To: Wenyi Xie <xiewenyi2@huawei.com>; devel@edk2.groups.io; Wang, Jian
J <jian.j.wang@intel.com>
Cc: songdongkuang@huawei.com
Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
MdeModulePkg/UefiSortLib:Add UefiSortLib unit test

-----Original Message-----
From: Wenyi Xie <xiewenyi2@huawei.com>
Sent: Thursday, July 29, 2021 4:01 PM
To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu,
Hao A <hao.a.wu@intel.com>
Cc: songdongkuang@huawei.com; xiewenyi2@huawei.com
Subject: [PATCH EDK2 v2 1/1] MdeModulePkg/UefiSortLib:Add UefiSortLib
unit test

Adding two unit test case for UefiSortLib. One is a test on sorting an
array of
UINT32 by using PerformQuickSort, another is a test on comparing the
same buffer by using StringCompare.

Thanks.
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Sorry, I found that there are a couple of coding format style check failures when merging the patch.
Could you help to resolve them and then create a test pull request on the GitHub for verification? Thanks in advance.
(I think you can take the case under MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/ for reference.)

Error details:
2021-08-02T01:09:28.5955820Z ##[section]Starting: Build and Test MdeModulePkg IA32,X64,ARM,AARCH64,RISCV64
2021-08-02T01:09:28.5960733Z ==============================================================================
2021-08-02T01:09:28.5961018Z Task : Command Line
2021-08-02T01:09:28.5961258Z Description : Run a command line with arguments
2021-08-02T01:09:28.5961502Z Version : 1.1.3
2021-08-02T01:09:28.5961897Z Author : Microsoft Corporation
2021-08-02T01:09:28.5962259Z Help : [More Information](https://go.microsoft.com/fwlink/?LinkID=613735)
2021-08-02T01:09:28.5962678Z ==============================================================================
2021-08-02T01:09:29.3504946Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3510020Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3511551Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3513605Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3516798Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3517536Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3518178Z (node:3659) Warning: Use Cipheriv for counter mode of aes-256-ctr
2021-08-02T01:09:29.3545461Z [command]/opt/hostedtoolcache/Python/3.8.11/x64/bin/stuart_ci_build -c .pytool/CISettings.py -p MdeModulePkg -t RELEASE,NO-TARGET -a IA32,X64,ARM,AARCH64,RISCV64 TOOL_CHAIN_TAG=GCC5
2021-08-02T01:09:29.3546707Z SECTION - Init SDE
2021-08-02T01:09:29.3547571Z WARNING - Using Pip Tools based BaseTools
2021-08-02T01:09:29.3548395Z SECTION - Loading Plugins
2021-08-02T01:09:29.3549212Z SECTION - Start Invocable Tool
2021-08-02T01:09:29.3550018Z SECTION - Getting Environment
2021-08-02T01:09:29.3550794Z SECTION - Loading plugins
2021-08-02T01:09:29.3551675Z SECTION - Building MdeModulePkg Package
2021-08-02T01:09:29.3552693Z PROGRESS - --Running MdeModulePkg: EccCheck Test NO-TARGET --
2021-08-02T01:09:33.0697006Z ERROR -
2021-08-02T01:09:33.0698018Z ERROR - EFI coding style error
2021-08-02T01:09:33.0698748Z ERROR - *Error code: 5007
2021-08-02T01:09:33.0700631Z ERROR - *There should be no initialization of a variable as part of its declaration
2021-08-02T01:09:33.0702675Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0703421Z ERROR - *Line number: 77
2021-08-02T01:09:33.0703929Z ERROR - *Variable Name: TestCount
2021-08-02T01:09:33.0715684Z ERROR -
2021-08-02T01:09:33.0716634Z ERROR - EFI coding style error
2021-08-02T01:09:33.0717372Z ERROR - *Error code: 5007
2021-08-02T01:09:33.0718214Z ERROR - *There should be no initialization of a variable as part of its declaration
2021-08-02T01:09:33.0719195Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0720631Z ERROR - *Line number: 78
2021-08-02T01:09:33.0721451Z ERROR - *Variable Name: TestBuffer
2021-08-02T01:09:33.0722114Z ERROR -
2021-08-02T01:09:33.0724953Z ERROR - EFI coding style error
2021-08-02T01:09:33.0725713Z ERROR - *Error code: 5007
2021-08-02T01:09:33.0726562Z ERROR - *There should be no initialization of a variable as part of its declaration
2021-08-02T01:09:33.0727526Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0728345Z ERROR - *Line number: 79
2021-08-02T01:09:33.0729046Z ERROR - *Variable Name: TestResult
2021-08-02T01:09:33.0729694Z ERROR -
2021-08-02T01:09:33.0732496Z ERROR - EFI coding style error
2021-08-02T01:09:33.0733224Z ERROR - *Error code: 7001
2021-08-02T01:09:33.0734114Z ERROR - *There should be no use of int, unsigned, char, void, long in any .c, .h or .asl files
2021-08-02T01:09:33.0737620Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0739185Z ERROR - *Line number: 117
2021-08-02T01:09:33.0741301Z ERROR - *[main] Return type int
2021-08-02T01:09:33.0744099Z ERROR -
2021-08-02T01:09:33.0744829Z ERROR - EFI coding style error
2021-08-02T01:09:33.0745459Z ERROR - *Error code: 7001
2021-08-02T01:09:33.0746821Z ERROR - *There should be no use of int, unsigned, char, void, long in any .c, .h or .asl files
2021-08-02T01:09:33.0747774Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0749076Z ERROR - *Line number: 117
2021-08-02T01:09:33.0749713Z ERROR - *Parameter argc
2021-08-02T01:09:33.0750999Z ERROR -
2021-08-02T01:09:33.0751607Z ERROR - EFI coding style error
2021-08-02T01:09:33.0754599Z ERROR - *Error code: 7001
2021-08-02T01:09:33.0761175Z ERROR - *There should be no use of int, unsigned, char, void, long in any .c, .h or .asl files
2021-08-02T01:09:33.0762167Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0763160Z ERROR - *Line number: 117
2021-08-02T01:09:33.0763741Z ERROR - *Parameter argv
2021-08-02T01:09:33.0764290Z ERROR -
2021-08-02T01:09:33.0764843Z ERROR - EFI coding style error
2021-08-02T01:09:33.0766810Z ERROR - *Error code: 8006
2021-08-02T01:09:33.0767819Z ERROR - *Function name does not follow the rules: 1. First character should be upper case 2. Must contain lower case characters 3. No white space characters
2021-08-02T01:09:33.0768881Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0769619Z ERROR - *Line number: 181
2021-08-02T01:09:33.0773329Z ERROR - *The function name [main] does not follow the rules
2021-08-02T01:09:33.0791935Z ERROR -
2021-08-02T01:09:33.0793865Z ERROR - EFI coding style error
2021-08-02T01:09:33.0794555Z ERROR - *Error code: 9002
2021-08-02T01:09:33.0795350Z ERROR - *The function headers should follow Doxygen special documentation blocks in section 2.3.5
2021-08-02T01:09:33.0796271Z ERROR - *file: //home/vsts/work/1/s/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
2021-08-02T01:09:33.0796985Z ERROR - *Line number: 178
2021-08-02T01:09:33.0797610Z ERROR - *No doxygen tags in comment
2021-08-02T01:09:33.0798208Z ERROR -
2021-08-02T01:09:33.0798859Z ERROR - --->Test Failed: EccCheck Test NO-TARGET returned 1
2021-08-02T01:09:33.0807351Z PROGRESS - --Running MdeModulePkg: Dsc Complete Check Test NO-TARGET --
2021-08-02T01:09:33.1524790Z PROGRESS - --->Test Success: Dsc Complete Check Test NO-TARGET
2021-08-02T01:09:33.1536004Z PROGRESS - --Running MdeModulePkg: Char Encoding Check Test NO-TARGET --
2021-08-02T01:09:33.6280380Z PROGRESS - --->Test Success: Char Encoding Check Test NO-TARGET
2021-08-02T01:09:33.6289076Z PROGRESS - --Running MdeModulePkg: License Check Test NO-TARGET --
2021-08-02T01:09:33.6495642Z PROGRESS - --->Test Success: License Check Test NO-TARGET
2021-08-02T01:09:33.6508125Z PROGRESS - --Running MdeModulePkg: Compiler Plugin RELEASE --
2021-08-02T01:09:33.6785178Z PROGRESS - Start time: 2021-08-02 01:09:33.677952
2021-08-02T01:09:33.6787716Z PROGRESS - Setting up the Environment
2021-08-02T01:09:33.7725244Z PROGRESS - Running Pre Build
2021-08-02T01:09:33.7740357Z PROGRESS - Running Build RELEASE
2021-08-02T01:17:18.5479885Z PROGRESS - Running Post Build
2021-08-02T01:17:18.5511807Z PROGRESS - End time: 2021-08-02 01:17:18.549974 Total time Elapsed: 0:07:44
2021-08-02T01:17:18.5512997Z PROGRESS - --->Test Success: Compiler Plugin RELEASE
2021-08-02T01:17:18.5519284Z PROGRESS - --Running MdeModulePkg: Library Class Check Test NO-TARGET --
2021-08-02T01:17:18.5639216Z PROGRESS - --->Test Success: Library Class Check Test NO-TARGET
2021-08-02T01:17:18.5648791Z PROGRESS - --Running MdeModulePkg: Dependency Check Test NO-TARGET --
2021-08-02T01:17:18.7306611Z PROGRESS - --->Test Success: Dependency Check Test NO-TARGET
2021-08-02T01:17:18.7316162Z PROGRESS - --Running MdeModulePkg: Spell Check Test NO-TARGET --
2021-08-02T01:17:23.7922166Z WARNING - --->Test Skipped: in plugin! Spell Check Test NO-TARGET
2021-08-02T01:17:23.7934112Z PROGRESS - --Running MdeModulePkg: Guid Check Test NO-TARGET --
2021-08-02T01:17:27.1928850Z PROGRESS - --->Test Success: Guid Check Test NO-TARGET
2021-08-02T01:17:27.1939129Z PROGRESS - --Running MdeModulePkg: Host Unit Test Dsc Complete Check Test NO-TARGET --
2021-08-02T01:17:27.3703640Z PROGRESS - --->Test Success: Host Unit Test Dsc Complete Check Test NO-TARGET
2021-08-02T01:17:27.3731676Z ERROR - Overall Build Status: Error
2021-08-02T01:17:27.3733283Z PROGRESS - There were 1 failures out of 10 attempts
2021-08-02T01:17:27.3734501Z SECTION - Summary
2021-08-02T01:17:27.3735613Z ERROR - Error
2021-08-02T01:17:27.4159842Z ##[error]/opt/hostedtoolcache/Python/3.8.11/x64/bin/stuart_ci_build failed with return code: 1
2021-08-02T01:17:27.4173134Z ##[error]/opt/hostedtoolcache/Python/3.8.11/x64/bin/stuart_ci_build failed with error: /opt/hostedtoolcache/Python/3.8.11/x64/bin/stuart_ci_build failed with return code: 1
2021-08-02T01:17:27.4178252Z ##[section]Finishing: Build and Test MdeModulePkg IA32,X64,ARM,AARCH64,RISCV64

Best Regards,
Hao Wu



I will wait a couple days before merging to see if any additional comment
from other reviewers.

Best Regards,
Hao Wu



Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
---
MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 6 +
MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.inf |
32
++++
MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c | 188
++++++++++++++++++++
3 files changed, 226 insertions(+)

diff --git a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
index 4da4692c8451..c9ec835df65d 100644
--- a/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
+++ b/MdeModulePkg/Test/MdeModulePkgHostTest.dsc
@@ -41,3 +41,9 @@ [Components]
<PcdsFixedAtBuild>

gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDis
able|TRUE
}
+
+ MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.inf {
+ <LibraryClasses>
+ UefiSortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
+
+ DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+ }
diff --git
a/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.inf
b/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.inf
new file mode 100644
index 000000000000..85d8dcd69619
--- /dev/null
+++
b/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.in
+++ f
@@ -0,0 +1,32 @@
+## @file
+# This is a unit test for the UefiSortLib.
+#
+# Copyright (C) Huawei Technologies Co., Ltd. All rights reserved #
+SPDX-License-Identifier: BSD-2-Clause-Patent ##
+
+[Defines]
+ INF_VERSION = 0x00010017
+ BASE_NAME = UefiSortLibUnitTest
+ FILE_GUID = 271337A3-0D79-BA3E-BC03-714E518E3B1B
+ VERSION_STRING = 1.0
+ MODULE_TYPE = HOST_APPLICATION
+
+#
+# The following information is for reference only and not required by
+the
build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64
+#
+
+[Sources]
+ UefiSortLibUnitTest.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
+
+[LibraryClasses]
+ UnitTestLib
+ DebugLib
+ UefiSortLib
diff --git
a/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
b/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
new file mode 100644
index 000000000000..71f30d8b9f7f
--- /dev/null
+++ b/MdeModulePkg/Library/UefiSortLib/UnitTest/UefiSortLibUnitTest.c
@@ -0,0 +1,188 @@
+/** @file
+ Unit tests of the UefiSortLib
+
+ Copyright (C) Huawei Technologies Co., Ltd. All rights reserved
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <stdio.h>
+#include <string.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include <Uefi.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+
+#include <Library/UnitTestLib.h>
+#include <Library/SortLib.h>
+
+#define UNIT_TEST_APP_NAME "UefiSortLib Unit Tests"
+#define UNIT_TEST_APP_VERSION "1.0"
+
+#define TEST_ARRAY_SIZE_9 9
+
+/**
+ The function is called by PerformQuickSort to compare int values.
+
+ @param[in] Left The pointer to first buffer.
+ @param[in] Right The pointer to second buffer.
+
+ @retval 0 Buffer1 equal to Buffer2.
+ @return <0 Buffer1 is less than Buffer2.
+ @return >0 Buffer1 is greater than Buffer2.
+
+**/
+INTN
+EFIAPI
+TestCompareFunction (
+ IN CONST VOID *Left,
+ IN CONST VOID *Right
+ )
+{
+ if (*(UINT32*)Right > *(UINT32*)Left) {
+ return 1;
+ } else if (*(UINT32*)Right < *(UINT32*)Left) {
+ return -1;
+ }
+
+ return 0;
+}
+
+/**
+ Unit test for PerformQuickSort () API of the UefiSortLib.
+
+ @param[in] Context [Optional] An optional parameter that enables:
+ 1) test-case reuse with varied parameters and
+ 2) test-case re-entry for Target tests that need a
+ reboot. This parameter is a VOID* and it is the
+ responsibility of the test author to ensure that the
+ contents are well understood by all test cases that may
+ consume it.
+
+ @retval UNIT_TEST_PASSED The Unit test has completed and the
test
+ case was successful.
+ @retval UNIT_TEST_ERROR_TEST_FAILED A test case assertion has
failed.
+**/
+UNIT_TEST_STATUS
+EFIAPI
+SortUINT32ArrayShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ UINTN TestCount = TEST_ARRAY_SIZE_9;
+ UINT32 TestBuffer[TEST_ARRAY_SIZE_9] = {1, 2, 3, 4, 5, 6, 7 ,8, 9};
+ UINT32 TestResult[TEST_ARRAY_SIZE_9] = {9, 8, 7, 6, 5, 4, 3, 2, 1};
+
+ PerformQuickSort (TestBuffer, TestCount, sizeof (UINT32),
+ (SORT_COMPARE)TestCompareFunction);
+ UT_ASSERT_MEM_EQUAL (TestBuffer, TestResult, sizeof (UINT32) *
+ TEST_ARRAY_SIZE_9);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Unit test for StringCompare () API of the UefiSortLib.
+
+ @param[in] Context [Optional] An optional parameter that enables:
+ 1) test-case reuse with varied parameters and
+ 2) test-case re-entry for Target tests that need a
+ reboot. This parameter is a VOID* and it is the
+ responsibility of the test author to ensure that the
+ contents are well understood by all test cases that may
+ consume it.
+
+ @retval UNIT_TEST_PASSED The Unit test has completed and the
test
+ case was successful.
+ @retval UNIT_TEST_ERROR_TEST_FAILED A test case assertion has
failed.
+**/
+UNIT_TEST_STATUS
+EFIAPI
+CompareSameBufferShouldSucceed (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ INTN retval;
+ CONST CHAR16* TestBuffer[] = { L"abcdefg" };
+
+ retval = StringCompare (TestBuffer, TestBuffer); UT_ASSERT_TRUE
+ (retval == 0);
+
+ return UNIT_TEST_PASSED;
+}
+
+/**
+ Initialze the unit test framework, suite, and unit tests for the
+ UefiSortLib and run the UefiSortLib unit test.
+
+ @retval EFI_SUCCESS All test cases were dispatched.
+ @retval EFI_OUT_OF_RESOURCES There are not enough resources
available to
+ initialize the unit tests.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+UnitTestingEntry (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UNIT_TEST_FRAMEWORK_HANDLE Framework;
+ UNIT_TEST_SUITE_HANDLE SortTests;
+
+ Framework = NULL;
+
+ DEBUG(( DEBUG_INFO, "%a v%a\n", UNIT_TEST_APP_NAME,
+ UNIT_TEST_APP_VERSION ));
+
+ //
+ // Start setting up the test framework for running the tests.
+ //
+ Status = InitUnitTestFramework (&Framework, UNIT_TEST_APP_NAME,
+ gEfiCallerBaseName, UNIT_TEST_APP_VERSION); if (EFI_ERROR (Status))
{
+ DEBUG ((DEBUG_ERROR, "Failed in InitUnitTestFramework. Status
= %r\n", Status));
+ goto EXIT;
+ }
+
+ //
+ // Populate the UefiSortLib Unit Test Suite.
+ //
+ Status = CreateUnitTestSuite (&SortTests, Framework, "UefiSortLib
+ Sort Tests", "UefiSortLib.SortLib", NULL, NULL); if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for
+ UefiSortLib
API Tests\n"));
+ Status = EFI_OUT_OF_RESOURCES;
+ goto EXIT;
+ }
+
+ //
+ //
+ --------------Suite--------Description------------Name--------------
+ Fu
+ nction----------------Pre---Post---Context-----------
+ //
+ AddTestCase (SortTests, "Sort the Array", "Sort",
SortUINT32ArrayShouldSucceed, NULL, NULL, NULL);
+ AddTestCase (SortTests, "Compare the Buffer", "Compare",
CompareSameBufferShouldSucceed, NULL, NULL, NULL);
+
+ //
+ // Execute the tests.
+ //
+ Status = RunAllTestSuites (Framework);
+
+EXIT:
+ if (Framework) {
+ FreeUnitTestFramework (Framework);
+ }
+
+ return Status;
+}
+
+/**
+ Standard POSIX C entry point for host based unit test execution.
+**/
+int
+main (
+ int argc,
+ char *argv[]
+ )
+{
+ return UnitTestingEntry ();
+}
--
2.20.1.windows.1




Re: [Patch] BaseTools: use shutil.copyfile instead shutil.copy2

Yuwei Chen
 

Reviewed-by: Yuwei Chen<yuwei.chen@intel.com>

-----Original Message-----
From: Feng, Bob C <bob.c.feng@intel.com>
Sent: Wednesday, July 28, 2021 7:45 PM
To: devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Chen, Christine
<yuwei.chen@intel.com>
Subject: [Patch] BaseTools: use shutil.copyfile instead shutil.copy2

In Split tool, the copy file actions only need to copy file content but not need
to copy file metadata.

copy2() copies the file metadata that causes split unit test failed under edk2-
basetools CI environment.

So this patch changes the call of copy2() to copyfile().

Signed-off-by: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/Split/Split.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/BaseTools/Source/Python/Split/Split.py
b/BaseTools/Source/Python/Split/Split.py
index e223a72a94..e70d5c22c4 100644
--- a/BaseTools/Source/Python/Split/Split.py
+++ b/BaseTools/Source/Python/Split/Split.py
@@ -146,18 +146,18 @@ def splitFile(inputfile, position, outputdir=None,
outputfile1=None, outputfile2
logger.error("Can't make dir: %s" % outputfolder) raise(e) if
position <= 0: if outputfile2 != os.path.abspath(inputfile):-
shutil.copy2(os.path.abspath(inputfile), outputfile2)+
shutil.copyfile(os.path.abspath(inputfile), outputfile2) with
open(outputfile1, "wb") as fout: fout.write(b'') else:
inputfilesize = getFileSize(inputfile) if position >= inputfilesize: if
outputfile1 != os.path.abspath(inputfile):-
shutil.copy2(os.path.abspath(inputfile), outputfile1)+
shutil.copyfile(os.path.abspath(inputfile), outputfile1) with
open(outputfile2, "wb") as fout: fout.write(b'') else: try:
tempdir = tempfile.mkdtemp()@@ -169,12 +169,12 @@ def splitFile(inputfile,
position, outputdir=None, outputfile1=None, outputfile2
fout1.write(content1) content2 =
fin.read(inputfilesize - position) with open(tempfile2, "wb") as
fout2: fout2.write(content2)- shutil.copy2(tempfile1,
outputfile1)- shutil.copy2(tempfile2, outputfile2)+
shutil.copyfile(tempfile1, outputfile1)+ shutil.copyfile(tempfile2,
outputfile2) except Exception as e: logger.error("Split file
failed") raise(e) finally: if os.path.exists(tempdir):--
2.29.1.windows.1


Re: ArmVirt and Self-Updating Code

Marvin Häuser
 

01.08.2021 18:33:47 Ard Biesheuvel <ardb@kernel.org>:

On Sat, 31 Jul 2021 at 21:08, Marvin Häuser <mhaeuser@posteo.de> wrote:
On 23.07.21 16:34, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 16:27, Marvin Häuser <mhaeuser@posteo.de> wrote:
On 23.07.21 16:09, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 12:47, Marvin Häuser <mhaeuser@posteo.de> wrote:
...
Do you maybe have one final comment regarding that second question,
please? :)
The RELA section is not converted into PE/COFF relocations. This would
not achieve a lot, given that no prior PE/COFF loader exists to
process them. There is a snippet of asm code in the startup code that
processes the R_AARCH64_RELATIVE relocation entries before calling
into C code.
I searched for said ASM code till my fingers fell asleep and at last
found this:
https://github.com/tianocore/edk2/commit/b16fd231f6d8124fa05a0f086840934b8709faf9#diff-3d563cc4775c7720900f4895bf619eed06291044aaa277fcc57eddc7618351a1L12-R148
If I understand the commit message correctly, it is basically "pray the
C code does not use globals at all", which is fair enough, so maybe I
should document this in my proposed new library? I trust that this is
enough of a constraint for both ARM and AArch64, because I do not know
them at all.
The C code can use globals, but not global pointer variables. But you
are right, this is not very robust at all.
Right... Will document for my PE library.

What worries me is that StandaloneMmCore has no such ASM entry point at
all and instead it's just executing C directly. Also, it is not passed
the "-fno-jump-tables" flag that is commented to be important in the
commit linked above.
This is because the StandaloneMmCore is built with -fpie, which
already implies -fno-jump-tables, although I suppose this may not
offer complete coverage for BASE libraries that are pulled into the
link.
Ah okay, thanks. Out of curiosity of how ARM implements PIE, and how StMmCore self-relocation can work *after* the PE/COFF section permissions have been applied with .got merged into .text (i.e. read-only), I checked the GCC5 "DLL" with readelf and found many relocations into the .text section. I have no idea how any of this works, and no idea where to find out, but as it apparently does, I might just update the PE calls and call it a day. I cannot test anything either because there is no QEMU code for StMmCore I can find. :(

Thanks for your tireless replies!

Best regards,
Marvin


Best regards,
Marvin

This also gives us the guarantee that no GOT indirections are
dereferenced, given that our asm code simply does not do that.

Let's drop "GOT" and make it "any instruction that requires prior
relocation to function correctly".
The thing to keep in mind here is that R_AARCH64_RELATIVE relocations
never target instructions, but only memory locations that carry
absolute addresses. This could be locations in .rodata or .data
(global vars carrying pointer values), or GOT entries.

Correct. And this works really well for shared libraries, where all
text and data sections can be shared between processes, as they will
not be modified by the loader. All locations targeted by relocations
will be nicely lumped together in the GOT.
However, for bare metal style programs, there is no sharing, and there
is no advantage to lumping anything together. It is much better to use
relative references where possible, and simply apply relocations
wherever needed across the text and data sections,

The GOT is a special data structure used for implicit variable
accesses, i.e., global vars used in the code. Statically initialized
pointer variables are the other category, which are not code, and for
which the same considerations do not apply, given that the right value
simply needs to be stored in the variable before the program starts.

The selection of 'code model' as it is called is controlled by GCC's
-mcmodel= argument, which defaults to 'small' on AArch64, regardless
of whether you use PIC/PIE or not.
Aha, makes sense, thanks!
Best regards,
Marvin


Re: [PATCH v7 00/11] Secure Boot default keys

Ard Biesheuvel
 

On Fri, 30 Jul 2021 at 12:23, Grzegorz Bernacki <gjb@semihalf.com> wrote:

This patchset adds support for initialization of default
Secure Boot variables based on keys content embedded in
flash binary. This feature is active only if Secure Boot
is enabled and DEFAULT_KEY is defined. The patchset
consist also application to enroll keys from default
variables and secure boot menu change to allow user
to reset key content to default values.
Discussion on design can be found at:
https://edk2.groups.io/g/rfc/topic/82139806#600

Built with:
GCC
- RISC-V (U500, U540) [requires fixes in dsc to build]
- Intel (Vlv2TbltDevicePkg (X64/IA32), Quark, MinPlatformPkg,
EmulatorPkg (X64), Bhyve, OvmfPkg (X64/IA32))
- ARM (Sgi75,SbsaQemu,DeveloperBox, RPi3/RPi4)

RISC-V, Quark, Vlv2TbltDevicePkg, Bhyve requires additional fixes to be built,
will be post on edk2 maillist later

VS2019
- Intel (OvmfPkgX64)

Test with:
GCC5/RPi4
VS2019/OvmfX64 (requires changes to enable feature)

Tests:
1. Try to enroll key in incorrect format.
2. Enroll with only PKDefault keys specified.
3. Enroll with all keys specified.
4. Enroll when keys are enrolled.
5. Reset keys values.
6. Running signed & unsigned app after enrollment.

Changes since v1:
- change names:
SecBootVariableLib => SecureBootVariableLib
SecBootDefaultKeysDxe => SecureBootDefaultKeysDxe
SecEnrollDefaultKeysApp => EnrollFromDefaultKeysApp
- change name of function CheckSetupMode to GetSetupMode
- remove ShellPkg dependecy from EnrollFromDefaultKeysApp
- rebase to master

Changes since v2:
- fix coding style for functions headers in SecureBootVariableLib.h
- add header to SecureBootDefaultKeys.fdf.inc
- remove empty line spaces in SecureBootDefaultKeysDxe files
- revert FAIL macro in EnrollFromDefaultKeysApp
- remove functions duplicates and add SecureBootVariableLib
to platforms which used it

Changes since v3:
- move SecureBootDefaultKeys.fdf.inc to ArmPlatformPkg
- leave duplicate of CreateTimeBasedPayload in PlatformVarCleanupLib
- fix typo in guid description

Changes since v4:
- reorder patches to make it bisectable
- split commits related to more than one platform
- move edk2-platform commits to separate patchset

Changes since v5:
- split SecureBootVariableLib into SecureBootVariableLib and
SecureBootVariableProvisionLib

Changes since v6:
- fix problems found by CI
- add correct modules to SecurityPkg.dsc
- update SecurityPkg.dec
- fix coding style issues
This still generates CI errors:

https://github.com/tianocore/edk2/pull/1850

Note that you can create PRs against tianocore/edk2 directly from your
own branch, which will result in the CI checks to be performed on the
code, without your branch being merged even if all checks pass (that
requires the push label which only maintainers can set)


NOTE: edk2-platform has not been changed and v6 platform patches
are still valid

Grzegorz Bernacki (11):
SecurityPkg: Create SecureBootVariableLib.
SecurityPkg: Create library for enrolling Secure Boot variables.
ArmVirtPkg: add SecureBootVariableLib class resolution
OvmfPkg: add SecureBootVariableLib class resolution
EmulatorPkg: add SecureBootVariableLib class resolution
SecurityPkg: Remove duplicated functions from SecureBootConfigDxe.
ArmPlatformPkg: Create include file for default key content.
SecurityPkg: Add SecureBootDefaultKeysDxe driver
SecurityPkg: Add EnrollFromDefaultKeys application.
SecurityPkg: Add new modules to Security package.
SecurityPkg: Add option to reset secure boot keys.

SecurityPkg/SecurityPkg.dec | 22 +
ArmVirtPkg/ArmVirt.dsc.inc | 2 +
EmulatorPkg/EmulatorPkg.dsc | 2 +
OvmfPkg/Bhyve/BhyveX64.dsc | 2 +
OvmfPkg/OvmfPkgIa32.dsc | 2 +
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +
OvmfPkg/OvmfPkgX64.dsc | 2 +
SecurityPkg/SecurityPkg.dsc | 9 +-
SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.inf | 48 ++
SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf | 80 +++
SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.inf | 80 +++
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf | 3 +
SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.inf | 46 ++
SecurityPkg/Include/Library/SecureBootVariableLib.h | 153 ++++++
SecurityPkg/Include/Library/SecureBootVariableProvisionLib.h | 134 +++++
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigNvData.h | 2 +
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig.vfr | 6 +
SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c | 115 +++++
SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c | 510 ++++++++++++++++++++
SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c | 482 ++++++++++++++++++
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 344 ++++++-------
SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c | 69 +++
ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc | 70 +++
SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni | 17 +
SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.uni | 16 +
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigStrings.uni | 4 +
SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.uni | 16 +
27 files changed, 2049 insertions(+), 189 deletions(-)
create mode 100644 SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.inf
create mode 100644 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf
create mode 100644 SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.inf
create mode 100644 SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.inf
create mode 100644 SecurityPkg/Include/Library/SecureBootVariableLib.h
create mode 100644 SecurityPkg/Include/Library/SecureBootVariableProvisionLib.h
create mode 100644 SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c
create mode 100644 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
create mode 100644 SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c
create mode 100644 SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c
create mode 100644 ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc
create mode 100644 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni
create mode 100644 SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.uni
create mode 100644 SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.uni

--
2.25.1


Re: [edk2-platforms PATCH 0/7] Marvell ACS improvements

Ard Biesheuvel
 

On Mon, 19 Jul 2021 at 11:30, Marcin Wojtas <mw@semihalf.com> wrote:

Hi,

This new series comes with the remaining improvements that allow
the ACS3.0 test suite to pass the SBSA/FWTS/SCT to the
maximum non-HW related extent. Missing _STA methods
and DBG2 description are added to the ACPI tables.
Moreover all platforms start using the maintained
MonotonicCounterRuntimeDxe. Also a build fix is added
for the VariablePolicyHelperLib resolution, that is
required after the recent changes in edk2.
Last but not least the SMBIOS Type0 description
is updated with the actual EDK2 firmare vendor and version
strings.

More details can be found in the commit logs.
The patchest is publicly available in the github:
https://github.com/semihalf-wojtas-marcin/edk2-platforms/commits/marvell-acs-r20210719

Best regards,
Marcin


Grzegorz Bernacki (2):
Marvell: Armada7k8k: Add missing VariablePolicyHelperLib resolution
Marvell: Armada7k8k/OcteonTx: Switch to MonotonicCounterRuntimeDxe

Marcin Wojtas (5):
Marvell: Armada7k8k/OcteonTx: Add missing _STA methods in ACPI tables
Marvell/Cn913xDbA: AcpiTables: Introduce DBG2 table
SolidRun/Armada80x0McBin: AcpiTables: Introduce DBG2 table
Marvell/Drivers: SmbiosPlatformDxe: Update Type0 information
Marvell: Armada7k8k/OcteonTx: Bump firmware to "EDK2 SH 1.0" revision
Pushed as

578ec6f09a00..d84c0545f4b4

with the _STA changes removed.

Thanks,

Silicon/Marvell/Marvell.dec | 2 +
Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc | 4 +-
Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 8 +-
Silicon/Marvell/Armada7k8k/Armada7k8k.fdf | 2 +-
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin.inf | 1 +
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 2 +
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9130DbA.inf | 1 +
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA.inf | 1 +
Silicon/Marvell/Armada7k8k/AcpiTables/AcpiHeader.h | 2 +
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dbg2.h | 9 ++
Silicon/Marvell/Armada7k8k/AcpiTables/IcuInterrupts.h | 2 +
Silicon/Marvell/OcteonTx/AcpiTables/T91/AcpiHeader.h | 2 +
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dbg2.h | 9 ++
Silicon/Marvell/OcteonTx/AcpiTables/T91/IcuInterrupts.h | 2 +
Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 6 +-
Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db/Dsdt.asl | 56 +++++++++++
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db/Dsdt.asl | 76 ++++++++++++++
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dbg2.aslc | 74 ++++++++++++++
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl | 105 ++++++++++++++++++++
Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc | 2 -
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl | 12 +++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dbg2.aslc | 74 ++++++++++++++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dsdt.asl | 89 +++++++++++++++++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Spcr.aslc | 2 -
24 files changed, 530 insertions(+), 13 deletions(-)
create mode 100644 Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dbg2.h
create mode 100644 Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dbg2.h
create mode 100644 Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dbg2.aslc
create mode 100644 Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dbg2.aslc

--
2.29.0


Re: [edk2-platforms PATCH 2/7] Marvell: Armada7k8k/OcteonTx: Add missing _STA methods in ACPI tables

Ard Biesheuvel
 

On Fri, 30 Jul 2021 at 11:57, Marcin Wojtas <mw@semihalf.com> wrote:

Hi Ard,

czw., 29 lip 2021 o 11:58 Ard Biesheuvel <ardb@kernel.org> napisał(a):

On Thu, 29 Jul 2021 at 11:46, Marcin Wojtas <mw@semihalf.com> wrote:

Hi Ard,

pon., 19 lip 2021 o 17:06 Marcin Wojtas <mw@semihalf.com> napisał(a):

Hi Ard,

pon., 19 lip 2021 o 11:54 Ard Biesheuvel <ardb@kernel.org> napisał(a):

On Mon, 19 Jul 2021 at 11:31, Marcin Wojtas <mw@semihalf.com> wrote:

BBR 1.0 spec says that _STA is required for each device in DSDT or SSDT.
Fix that for all platforms with the Marvell SoC's.
Can we fix the BBR instead? If ACPI itself does not require _STA, BBR
should not require it either.

I consulted with ARM on the matter. SBBR has requirements of things
that are otherwise optional in UEFI/ACPI/SMBIOS. Also some OS's may
require that and I can see those methods in most of the other ACPI
source files in the edk2-platfoms tree. I think the BBR requirements
discussions can follow, but it would be great if this change can be
applied, so that no to block other development.
Do you have any feedback to the patchset and the _STA methods concerns?
Yes. I would like to understand why _STA methods are now mandated by BBR.
Understood. Providing an answer may not be immediate and may possibly
require further discussion on the SystemArchAC level.
How about we withdraw this single patch for now and process the
remaining ones?
Fair enough.

We would come back to the _STA subject, as soon as
there's more information available.
Thanks.





Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db/Dsdt.asl | 56 +++++++++++++++
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db/Dsdt.asl | 76 ++++++++++++++++++++
Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl | 72 +++++++++++++++++++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl | 12 ++++
Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dsdt.asl | 56 +++++++++++++++
5 files changed, 272 insertions(+)

diff --git a/Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db/Dsdt.asl b/Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db/Dsdt.asl
index 345c1e4dd6..88e38efeeb 100644
--- a/Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db/Dsdt.asl
+++ b/Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db/Dsdt.asl
@@ -20,21 +20,37 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x000) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU1)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x001) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU2)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x100) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x101) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}

Device (AHC0)
@@ -42,6 +58,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_HID, "LNRO001E") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CLS, Package (0x03) // _CLS: Class Code
{
0x01,
@@ -67,6 +87,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_HID, "MRVL0002") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -96,6 +120,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_HID, "MRVL0004") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -123,6 +151,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -142,6 +174,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -160,6 +196,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
{
Name (_HID, "MRVL0001") // _HID: Hardware ID
Name (_CID, "HISI0031") // _CID: Compatible ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_ADR, FixedPcdGet64(PcdSerialRegisterBase)) // _ADR: Address
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -186,6 +226,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
{
Name (_HID, "MRVL0100") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
@@ -208,6 +252,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf2000000 , 0x100000)
@@ -286,6 +334,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
{
Name (_HID, "PRP0001") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xF2760000, 0x7D)
@@ -312,6 +364,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA7K", 3)
Name (_SEG, 0x00) // _SEG: PCI Segment
Name (_BBN, 0x00) // _BBN: BIOS Bus Number
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_PRT, Package () // _PRT: PCI Routing Table
{
Package () { 0xFFFF, 0x0, 0x0, 0x40 },
diff --git a/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db/Dsdt.asl b/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db/Dsdt.asl
index 91401c74c8..77d3aebaf1 100644
--- a/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db/Dsdt.asl
+++ b/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db/Dsdt.asl
@@ -20,21 +20,37 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x000) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU1)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x001) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU2)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x100) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x101) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}

Device (AHC0)
@@ -42,6 +58,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "LNRO001E") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CLS, Package (0x03) // _CLS: Class Code
{
0x01,
@@ -67,6 +87,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "LNRO001E") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CLS, Package (0x03) // _CLS: Class Code
{
0x01,
@@ -92,6 +116,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0002") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -122,6 +150,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0004") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -151,6 +183,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -170,6 +206,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -189,6 +229,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x02) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -207,6 +251,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "MRVL0001") // _HID: Hardware ID
Name (_CID, "HISI0031") // _CID: Compatible ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_ADR, FixedPcdGet64(PcdSerialRegisterBase)) // _ADR: Address
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -233,6 +281,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "MRVL0100") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
@@ -251,6 +303,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf2000000 , 0x100000)
@@ -309,6 +365,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "MRVL0100") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
@@ -327,6 +387,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x01) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf4000000 , 0x100000)
@@ -385,6 +449,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "PRP0001") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xF2760000, 0x7D)
@@ -405,6 +473,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "PRP0001") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xF4760000, 0x7D)
@@ -431,6 +503,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_SEG, 0x00) // _SEG: PCI Segment
Name (_BBN, 0x00) // _BBN: BIOS Bus Number
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_PRT, Package () // _PRT: PCI Routing Table
{
Package () { 0xFFFF, 0x0, 0x0, 0x40 },
diff --git a/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl b/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl
index d26945d933..1ecbd0309c 100644
--- a/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl
+++ b/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl
@@ -19,21 +19,37 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x000) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU1)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x001) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU2)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x100) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x101) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}

Device (AHC0)
@@ -41,6 +57,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "LNRO001E") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CLS, Package (0x03) // _CLS: Class Code
{
0x01,
@@ -91,6 +111,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0002") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -122,6 +146,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0004") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -150,6 +178,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -169,6 +201,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -188,6 +224,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x02) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -206,6 +246,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "MRVL0001") // _HID: Hardware ID
Name (_CID, "HISI0031") // _CID: Compatible ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_ADR, FixedPcdGet64(PcdSerialRegisterBase)) // _ADR: Address
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -232,6 +276,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "MRVL0100") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
@@ -249,6 +297,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "MRVL0101") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
@@ -283,6 +335,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf2000000 , 0x100000)
@@ -322,6 +378,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x01) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf4000000 , 0x100000)
@@ -400,6 +460,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "PRP0001") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xF2760000, 0x7D)
@@ -420,6 +484,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
{
Name (_HID, "PRP0001") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xF4760000, 0x7D)
@@ -446,6 +514,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "ARMADA8K", 3)
Name (_SEG, 0x00) // _SEG: PCI Segment
Name (_BBN, 0x00) // _BBN: BIOS Bus Number
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_PRT, Package () // _PRT: PCI Routing Table
{
Package () { 0xFFFF, 0x0, 0x0, 0x40 },
diff --git a/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl
index 8377b13763..d6619e367b 100644
--- a/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl
+++ b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn9131DbA/Ssdt.asl
@@ -20,6 +20,10 @@ DefinitionBlock ("Cn9131DbASsdt.aml", "SSDT", 2, "MVEBU ", "CN9131", 3)
Name (_HID, "LNRO001E") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CLS, Package (0x03) // _CLS: Class Code
{
0x01,
@@ -45,6 +49,10 @@ DefinitionBlock ("Cn9131DbASsdt.aml", "SSDT", 2, "MVEBU ", "CN9131", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x02) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -63,6 +71,10 @@ DefinitionBlock ("Cn9131DbASsdt.aml", "SSDT", 2, "MVEBU ", "CN9131", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x01) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf4000000 , 0x100000)
diff --git a/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dsdt.asl b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dsdt.asl
index d76a2a902b..536df8ab4b 100644
--- a/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dsdt.asl
+++ b/Silicon/Marvell/OcteonTx/AcpiTables/T91/Cn913xDbA/Dsdt.asl
@@ -20,21 +20,37 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x000) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU1)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x001) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU2)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x100) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}
Device (CPU3)
{
Name (_HID, "ACPI0007" /* Processor Device */) // _HID: Hardware ID
Name (_UID, 0x101) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
}

Device (AHC0)
@@ -42,6 +58,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_HID, "LNRO001E") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CLS, Package (0x03) // _CLS: Class Code
{
0x01,
@@ -67,6 +87,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_HID, "MRVL0003") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -98,6 +122,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_HID, "MRVL0004") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -126,6 +154,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -145,6 +177,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_HID, "PNP0D10") // _HID: Hardware ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }

Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -163,6 +199,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
{
Name (_HID, "MRVL0001") // _HID: Hardware ID
Name (_CID, "HISI0031") // _CID: Compatible ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_ADR, FixedPcdGet64(PcdSerialRegisterBase)) // _ADR: Address
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
@@ -189,6 +229,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
{
Name (_HID, "MRVL0100") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
@@ -211,6 +255,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_HID, "MRVL0110") // _HID: Hardware ID
Name (_CCA, 0x01) // Cache-coherent controller
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xf2000000 , 0x100000)
@@ -289,6 +337,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
{
Name (_HID, "PRP0001") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xF2760000, 0x7D)
@@ -315,6 +367,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "MVEBU ", "CN9130", 3)
Name (_SEG, 0x00) // _SEG: PCI Segment
Name (_BBN, 0x00) // _BBN: BIOS Bus Number
Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
+ Method (_STA) // _STA: Device status
+ {
+ Return (0xF)
+ }
Name (_PRT, Package () // _PRT: PCI Routing Table
{
Package () { 0xFFFF, 0x0, 0x0, 0x40 },
--
2.29.0


Re: [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

Ard Biesheuvel
 

(correct Achin's email address, cc other replyers)

On Sun, 1 Aug 2021 at 18:36, Ard Biesheuvel <ardb@kernel.org> wrote:

On Fri, 30 Jul 2021 at 19:35, Sayanta Pattanayak
<sayanta.pattanayak@arm.com> wrote:

Introduce support to populate StMM boot data via DTS parsing.
Why? Don't we have FF-A manifests for this? I would expect the secure
partition manager to marshal this data into the appropriate format
when necessary.

The DTB is
passed as a boot argument by a binary of higer exception level.
Previously it was achieved by placing the boot data structure in a
shared buffer and the address of this shared buffer was passed by the
binary of higher exception level. Now either of the option can be used
for populating StMM boot info.

StMM boot information structure binding in device tree can be of following
prototype. Property values are not mentioned here.

bootarg {
compatible = "bootargs";
h_type = <..>;
h_version = <..>;
h_size = <..>;
h_attr = <..>;
sp_mem_base = <..>;
sp_mem_limit = <..>;
sp_image_base = <..>;
sp_stack_base = <..>;
sp_heap_base = <..>;
sp_ns_comm_buf_base = <..>;
sp_shared_buf_base = <..>;
sp_image_size = <..>;
sp_pcpu_stack_size = <..>;
sp_heap_size = <..>;
sp_ns_comm_buf_size = <..>;
sp_shared_buf_size = <..>;
num_sp_mem_regions = <..>;
num_cpus = <..>;
};

Addition of DTS supoort involves a dependency on FdtLib from EmbeddedPkg.

Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
I don't think we should apply this change. DT is not part of the
original SPM or current FF-A spec, right? So please fix this in the
S-EL1 component instead.


---
Link to github branch with this patch -
https://github.com/SayantaP-arm/edk2/tree/stmm-dts

StandaloneMmPkg/StandaloneMmPkg.dsc | 1 +
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 3 +
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 153 ++++++++++++++++++--
3 files changed, 143 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 0c45df95e2dd..e3a3a6ee3ba1 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -49,6 +49,7 @@
HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+ FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58ef4..0a2e519dd664 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -30,6 +30,7 @@
X64/StandaloneMmCoreEntryPoint.c

[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
StandaloneMmPkg/StandaloneMmPkg.dec
@@ -40,10 +41,12 @@
[LibraryClasses]
BaseLib
DebugLib
+ FdtLib

[LibraryClasses.AARCH64]
StandaloneMmMmuLib
ArmSvcLib
+ FdtLib

[Guids]
gMpInformationHobGuid
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 6c50f470aa35..cc09d75dac36 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MmramMemoryReserve.h>
#include <Guid/MpInformation.h>

+#include <libfdt.h>
#include <Library/ArmMmuLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/DebugLib.h>
@@ -45,33 +46,31 @@ STATIC CONST UINT32 mSpmMinorVerFfa = SPM_MINOR_VERSION_FFA;
PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT CpuDriverEntryPoint = NULL;

/**
- Retrieve a pointer to and print the boot information passed by privileged
- secure firmware.
+ Prints boot information.

- @param [in] SharedBufAddress The pointer memory shared with privileged
- firmware.
+ This function prints the boot information, which is passed by privileged
+ secure firmware through shared buffer or other mechanism.

+ @param [in] PayloadBootInfo Pointer to StandaloneMM Boot Info structure.
**/
-EFI_SECURE_PARTITION_BOOT_INFO *
-GetAndPrintBootinformation (
- IN VOID *SharedBufAddress
+VOID
+PrintBootinformation (
+ IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo
)
{
- EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
EFI_SECURE_PARTITION_CPU_INFO *PayloadCpuInfo;
UINTN Index;

- PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *) SharedBufAddress;

if (PayloadBootInfo == NULL) {
DEBUG ((DEBUG_ERROR, "PayloadBootInfo NULL\n"));
- return NULL;
+ return;
}

if (PayloadBootInfo->Header.Version != BOOT_PAYLOAD_VERSION) {
DEBUG ((DEBUG_ERROR, "Boot Information Version Mismatch. Current=0x%x, Expected=0x%x.\n",
PayloadBootInfo->Header.Version, BOOT_PAYLOAD_VERSION));
- return NULL;
+ return;
}

DEBUG ((DEBUG_INFO, "NumSpMemRegions - 0x%x\n", PayloadBootInfo->NumSpMemRegions));
@@ -96,7 +95,7 @@ GetAndPrintBootinformation (

if (PayloadCpuInfo == NULL) {
DEBUG ((DEBUG_ERROR, "PayloadCpuInfo NULL\n"));
- return NULL;
+ return;
}

for (Index = 0; Index < PayloadBootInfo->NumCpus; Index++) {
@@ -105,7 +104,7 @@ GetAndPrintBootinformation (
DEBUG ((DEBUG_INFO, "Flags - 0x%x\n", PayloadCpuInfo[Index].Flags));
}

- return PayloadBootInfo;
+ return;
}

/**
@@ -194,6 +193,119 @@ DelegatedEventLoop (
}
}

+/**
+ Populates StandAloneMM boot information structure.
+
+ This function receives dtb Address, where StMM Boot information specific
+ properties will be looked out to form the booting structure of type
+ EFI_SECURE_PARTITION_BOOT_INFO. At first, the properties for StandAloneMM
+ ConfigSize and Memory limit will be checked out. Boot information will
+ be stored at address (Memory Limit - ConfigSize). Thereafter all boot
+ information specific properties will be parsed and corresponding values
+ will be obtained.
+
+ @param [out] BootInfo Pointer, where Boot Info structure will be populated.
+ @param [in] DtbAddress Address of the Device tree from where Boot
+ information will be fetched.
+**/
+VOID
+PopulateBootinformation (
+ OUT EFI_SECURE_PARTITION_BOOT_INFO **BootInfo,
+ IN VOID *DtbAddress
+)
+{
+ INT32 Offset;
+ CONST UINT32 *Property;
+ CONST UINT64 *Property64;
+ UINT32 ConfigSize;
+ UINT64 SpMemLimit;
+ EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
+
+ Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "config-size");
+ if (Offset < 0) {
+ DEBUG ((DEBUG_WARN, "Total Config Size is not defined\n"));
+ } else {
+ Property = fdt_getprop (DtbAddress, Offset, "size", NULL);
+ if (Property) {
+ ConfigSize = fdt32_to_cpu (*Property);
+ DEBUG ((DEBUG_INFO, "stmm dtb config-size = 0x%x \n", ConfigSize));
+ }
+ }
+
+ Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "bootargs");
+ if (Offset >= 0) {
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_mem_limit", NULL);
+ SpMemLimit = fdt64_to_cpu (*Property64);
+ }
+
+ if (SpMemLimit && ConfigSize)
+ PayloadBootInfo =
+ (EFI_SECURE_PARTITION_BOOT_INFO *)(SpMemLimit - ConfigSize);
+
+ if (PayloadBootInfo) {
+ PayloadBootInfo->SpMemLimit = SpMemLimit;
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_type", NULL);
+ PayloadBootInfo->Header.Type = (UINT8) fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_version", NULL);
+ PayloadBootInfo->Header.Version = (UINT8) fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_size", NULL);
+ PayloadBootInfo->Header.Size = (UINT8) fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_attr", NULL);
+ PayloadBootInfo->Header.Attr = fdt32_to_cpu(*Property);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_mem_base", NULL);
+ PayloadBootInfo->SpMemBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_image_base", NULL);
+ PayloadBootInfo->SpImageBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_stack_base", NULL);
+ PayloadBootInfo->SpStackBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_heap_base", NULL);
+ PayloadBootInfo->SpHeapBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_base", NULL);
+ PayloadBootInfo->SpNsCommBufBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_shared_buf_base", NULL);
+ PayloadBootInfo->SpSharedBufBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_image_size", NULL);
+ PayloadBootInfo->SpImageSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_pcpu_stack_size", NULL);
+ PayloadBootInfo->SpPcpuStackSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_heap_size", NULL);
+ PayloadBootInfo->SpHeapSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_size", NULL);
+ PayloadBootInfo->SpNsCommBufSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_shared_buf_size", NULL);
+ PayloadBootInfo->SpPcpuSharedBufSize = fdt64_to_cpu(*Property64);
+
+ Property = fdt_getprop (DtbAddress, Offset, "num_sp_mem_regions", NULL);
+ PayloadBootInfo->NumSpMemRegions = fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "num_cpus", NULL);
+ PayloadBootInfo->NumCpus = fdt32_to_cpu(*Property);
+
+ PayloadBootInfo->CpuInfo =
+ (EFI_SECURE_PARTITION_CPU_INFO *)((UINT64)PayloadBootInfo +
+ sizeof(EFI_SECURE_PARTITION_BOOT_INFO));
+ }
+
+ *BootInfo = PayloadBootInfo;
+
+ return;
+}
+
/**
Query the SPM version, check compatibility and return success if compatible.

@@ -313,6 +425,7 @@ _ModuleEntryPoint (
VOID *TeData;
UINTN TeDataSize;
EFI_PHYSICAL_ADDRESS ImageBase;
+ VOID *DtbAddress;

// Get Secure Partition Manager Version Information
Status = GetSpmVersion ();
@@ -320,12 +433,24 @@ _ModuleEntryPoint (
goto finish;
}

- PayloadBootInfo = GetAndPrintBootinformation (SharedBufAddress);
+ // In cookie1 the DTB address is passed. With reference to DTB, Boot
+ // info structure can be populated.
+ // If cookie1 doesn't have any value, then Boot info is copied from
+ // Sharedbuffer.
+ if (cookie1) {
+ DtbAddress = (void *)cookie1;
+ PopulateBootinformation (&PayloadBootInfo, DtbAddress);
+ } else {
+ PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *)SharedBufAddress;
+ }
+
if (PayloadBootInfo == NULL) {
Status = EFI_UNSUPPORTED;
goto finish;
}

+ PrintBootinformation (PayloadBootInfo);
+
// Locate PE/COFF File information for the Standalone MM core module
Status = LocateStandaloneMmCorePeCoffData (
(EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
--
2.17.1


Re: [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

Ard Biesheuvel
 

On Fri, 30 Jul 2021 at 19:35, Sayanta Pattanayak
<sayanta.pattanayak@arm.com> wrote:

Introduce support to populate StMM boot data via DTS parsing.
Why? Don't we have FF-A manifests for this? I would expect the secure
partition manager to marshall this data into the appropriate format
when necessary.

The DTB is
passed as a boot argument by a binary of higer exception level.
Previously it was achieved by placing the boot data structure in a
shared buffer and the address of this shared buffer was passed by the
binary of higher exception level. Now either of the option can be used
for populating StMM boot info.

StMM boot information structure binding in device tree can be of following
prototype. Property values are not mentioned here.

bootarg {
compatible = "bootargs";
h_type = <..>;
h_version = <..>;
h_size = <..>;
h_attr = <..>;
sp_mem_base = <..>;
sp_mem_limit = <..>;
sp_image_base = <..>;
sp_stack_base = <..>;
sp_heap_base = <..>;
sp_ns_comm_buf_base = <..>;
sp_shared_buf_base = <..>;
sp_image_size = <..>;
sp_pcpu_stack_size = <..>;
sp_heap_size = <..>;
sp_ns_comm_buf_size = <..>;
sp_shared_buf_size = <..>;
num_sp_mem_regions = <..>;
num_cpus = <..>;
};

Addition of DTS supoort involves a dependency on FdtLib from EmbeddedPkg.

Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
I don't think we should apply this change. DT is not part of the
original SPM or current FF-A spec, right? So please fix this in the
S-EL1 component instead.


---
Link to github branch with this patch -
https://github.com/SayantaP-arm/edk2/tree/stmm-dts

StandaloneMmPkg/StandaloneMmPkg.dsc | 1 +
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 3 +
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 153 ++++++++++++++++++--
3 files changed, 143 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 0c45df95e2dd..e3a3a6ee3ba1 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -49,6 +49,7 @@
HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+ FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58ef4..0a2e519dd664 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -30,6 +30,7 @@
X64/StandaloneMmCoreEntryPoint.c

[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
StandaloneMmPkg/StandaloneMmPkg.dec
@@ -40,10 +41,12 @@
[LibraryClasses]
BaseLib
DebugLib
+ FdtLib

[LibraryClasses.AARCH64]
StandaloneMmMmuLib
ArmSvcLib
+ FdtLib

[Guids]
gMpInformationHobGuid
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 6c50f470aa35..cc09d75dac36 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MmramMemoryReserve.h>
#include <Guid/MpInformation.h>

+#include <libfdt.h>
#include <Library/ArmMmuLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/DebugLib.h>
@@ -45,33 +46,31 @@ STATIC CONST UINT32 mSpmMinorVerFfa = SPM_MINOR_VERSION_FFA;
PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT CpuDriverEntryPoint = NULL;

/**
- Retrieve a pointer to and print the boot information passed by privileged
- secure firmware.
+ Prints boot information.

- @param [in] SharedBufAddress The pointer memory shared with privileged
- firmware.
+ This function prints the boot information, which is passed by privileged
+ secure firmware through shared buffer or other mechanism.

+ @param [in] PayloadBootInfo Pointer to StandaloneMM Boot Info structure.
**/
-EFI_SECURE_PARTITION_BOOT_INFO *
-GetAndPrintBootinformation (
- IN VOID *SharedBufAddress
+VOID
+PrintBootinformation (
+ IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo
)
{
- EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
EFI_SECURE_PARTITION_CPU_INFO *PayloadCpuInfo;
UINTN Index;

- PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *) SharedBufAddress;

if (PayloadBootInfo == NULL) {
DEBUG ((DEBUG_ERROR, "PayloadBootInfo NULL\n"));
- return NULL;
+ return;
}

if (PayloadBootInfo->Header.Version != BOOT_PAYLOAD_VERSION) {
DEBUG ((DEBUG_ERROR, "Boot Information Version Mismatch. Current=0x%x, Expected=0x%x.\n",
PayloadBootInfo->Header.Version, BOOT_PAYLOAD_VERSION));
- return NULL;
+ return;
}

DEBUG ((DEBUG_INFO, "NumSpMemRegions - 0x%x\n", PayloadBootInfo->NumSpMemRegions));
@@ -96,7 +95,7 @@ GetAndPrintBootinformation (

if (PayloadCpuInfo == NULL) {
DEBUG ((DEBUG_ERROR, "PayloadCpuInfo NULL\n"));
- return NULL;
+ return;
}

for (Index = 0; Index < PayloadBootInfo->NumCpus; Index++) {
@@ -105,7 +104,7 @@ GetAndPrintBootinformation (
DEBUG ((DEBUG_INFO, "Flags - 0x%x\n", PayloadCpuInfo[Index].Flags));
}

- return PayloadBootInfo;
+ return;
}

/**
@@ -194,6 +193,119 @@ DelegatedEventLoop (
}
}

+/**
+ Populates StandAloneMM boot information structure.
+
+ This function receives dtb Address, where StMM Boot information specific
+ properties will be looked out to form the booting structure of type
+ EFI_SECURE_PARTITION_BOOT_INFO. At first, the properties for StandAloneMM
+ ConfigSize and Memory limit will be checked out. Boot information will
+ be stored at address (Memory Limit - ConfigSize). Thereafter all boot
+ information specific properties will be parsed and corresponding values
+ will be obtained.
+
+ @param [out] BootInfo Pointer, where Boot Info structure will be populated.
+ @param [in] DtbAddress Address of the Device tree from where Boot
+ information will be fetched.
+**/
+VOID
+PopulateBootinformation (
+ OUT EFI_SECURE_PARTITION_BOOT_INFO **BootInfo,
+ IN VOID *DtbAddress
+)
+{
+ INT32 Offset;
+ CONST UINT32 *Property;
+ CONST UINT64 *Property64;
+ UINT32 ConfigSize;
+ UINT64 SpMemLimit;
+ EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
+
+ Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "config-size");
+ if (Offset < 0) {
+ DEBUG ((DEBUG_WARN, "Total Config Size is not defined\n"));
+ } else {
+ Property = fdt_getprop (DtbAddress, Offset, "size", NULL);
+ if (Property) {
+ ConfigSize = fdt32_to_cpu (*Property);
+ DEBUG ((DEBUG_INFO, "stmm dtb config-size = 0x%x \n", ConfigSize));
+ }
+ }
+
+ Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "bootargs");
+ if (Offset >= 0) {
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_mem_limit", NULL);
+ SpMemLimit = fdt64_to_cpu (*Property64);
+ }
+
+ if (SpMemLimit && ConfigSize)
+ PayloadBootInfo =
+ (EFI_SECURE_PARTITION_BOOT_INFO *)(SpMemLimit - ConfigSize);
+
+ if (PayloadBootInfo) {
+ PayloadBootInfo->SpMemLimit = SpMemLimit;
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_type", NULL);
+ PayloadBootInfo->Header.Type = (UINT8) fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_version", NULL);
+ PayloadBootInfo->Header.Version = (UINT8) fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_size", NULL);
+ PayloadBootInfo->Header.Size = (UINT8) fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "h_attr", NULL);
+ PayloadBootInfo->Header.Attr = fdt32_to_cpu(*Property);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_mem_base", NULL);
+ PayloadBootInfo->SpMemBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_image_base", NULL);
+ PayloadBootInfo->SpImageBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_stack_base", NULL);
+ PayloadBootInfo->SpStackBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_heap_base", NULL);
+ PayloadBootInfo->SpHeapBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_base", NULL);
+ PayloadBootInfo->SpNsCommBufBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_shared_buf_base", NULL);
+ PayloadBootInfo->SpSharedBufBase = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_image_size", NULL);
+ PayloadBootInfo->SpImageSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_pcpu_stack_size", NULL);
+ PayloadBootInfo->SpPcpuStackSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_heap_size", NULL);
+ PayloadBootInfo->SpHeapSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_size", NULL);
+ PayloadBootInfo->SpNsCommBufSize = fdt64_to_cpu(*Property64);
+
+ Property64 = fdt_getprop (DtbAddress, Offset, "sp_shared_buf_size", NULL);
+ PayloadBootInfo->SpPcpuSharedBufSize = fdt64_to_cpu(*Property64);
+
+ Property = fdt_getprop (DtbAddress, Offset, "num_sp_mem_regions", NULL);
+ PayloadBootInfo->NumSpMemRegions = fdt32_to_cpu(*Property);
+
+ Property = fdt_getprop (DtbAddress, Offset, "num_cpus", NULL);
+ PayloadBootInfo->NumCpus = fdt32_to_cpu(*Property);
+
+ PayloadBootInfo->CpuInfo =
+ (EFI_SECURE_PARTITION_CPU_INFO *)((UINT64)PayloadBootInfo +
+ sizeof(EFI_SECURE_PARTITION_BOOT_INFO));
+ }
+
+ *BootInfo = PayloadBootInfo;
+
+ return;
+}
+
/**
Query the SPM version, check compatibility and return success if compatible.

@@ -313,6 +425,7 @@ _ModuleEntryPoint (
VOID *TeData;
UINTN TeDataSize;
EFI_PHYSICAL_ADDRESS ImageBase;
+ VOID *DtbAddress;

// Get Secure Partition Manager Version Information
Status = GetSpmVersion ();
@@ -320,12 +433,24 @@ _ModuleEntryPoint (
goto finish;
}

- PayloadBootInfo = GetAndPrintBootinformation (SharedBufAddress);
+ // In cookie1 the DTB address is passed. With reference to DTB, Boot
+ // info structure can be populated.
+ // If cookie1 doesn't have any value, then Boot info is copied from
+ // Sharedbuffer.
+ if (cookie1) {
+ DtbAddress = (void *)cookie1;
+ PopulateBootinformation (&PayloadBootInfo, DtbAddress);
+ } else {
+ PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *)SharedBufAddress;
+ }
+
if (PayloadBootInfo == NULL) {
Status = EFI_UNSUPPORTED;
goto finish;
}

+ PrintBootinformation (PayloadBootInfo);
+
// Locate PE/COFF File information for the Standalone MM core module
Status = LocateStandaloneMmCorePeCoffData (
(EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
--
2.17.1


Re: ArmVirt and Self-Updating Code

Ard Biesheuvel
 

On Sat, 31 Jul 2021 at 21:08, Marvin Häuser <mhaeuser@posteo.de> wrote:

On 23.07.21 16:34, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 16:27, Marvin Häuser <mhaeuser@posteo.de> wrote:


On 23.07.21 16:09, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 12:47, Marvin Häuser <mhaeuser@posteo.de> wrote:
On 23.07.21 12:13, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 11:55, Marvin Häuser <mhaeuser@posteo.de> wrote:
...
2) emit a GOT, which ends up being converted to PE/COFF Relocations (->
self-relocation), for global data that cannot be referenced relatively?
Is there any way to know/force that no symbol in GOT is accessed up
until the end of the self-relocation process?
Do you maybe have one final comment regarding that second question,
please? :)
The RELA section is not converted into PE/COFF relocations. This would
not achieve a lot, given that no prior PE/COFF loader exists to
process them. There is a snippet of asm code in the startup code that
processes the R_AARCH64_RELATIVE relocation entries before calling
into C code.
I searched for said ASM code till my fingers fell asleep and at last
found this:
https://github.com/tianocore/edk2/commit/b16fd231f6d8124fa05a0f086840934b8709faf9#diff-3d563cc4775c7720900f4895bf619eed06291044aaa277fcc57eddc7618351a1L12-R148

If I understand the commit message correctly, it is basically "pray the
C code does not use globals at all", which is fair enough, so maybe I
should document this in my proposed new library? I trust that this is
enough of a constraint for both ARM and AArch64, because I do not know
them at all.
The C code can use globals, but not global pointer variables. But you
are right, this is not very robust at all.


What worries me is that StandaloneMmCore has no such ASM entry point at
all and instead it's just executing C directly. Also, it is not passed
the "-fno-jump-tables" flag that is commented to be important in the
commit linked above.
This is because the StandaloneMmCore is built with -fpie, which
already implies -fno-jump-tables, although I suppose this may not
offer complete coverage for BASE libraries that are pulled into the
link.


Best regards,
Marvin

This also gives us the guarantee that no GOT indirections are
dereferenced, given that our asm code simply does not do that.

Let's drop "GOT" and make it "any instruction that requires prior
relocation to function correctly".
The thing to keep in mind here is that R_AARCH64_RELATIVE relocations
never target instructions, but only memory locations that carry
absolute addresses. This could be locations in .rodata or .data
(global vars carrying pointer values), or GOT entries.

It is not really a GOT. Actually, a GOT is undesirable, as it forces
global variables to be referenced via an absolute address, even when a
relative reference could be used.
Hmm, the GCC docs say a GOT is used for "all constant addresses" (I took
it as "absolute"?), it is kind of vague. I understood it this way:
1) no-pie emits relocations that can target the .text and .data sections
for instructions that embed and variables that hold an absolute address
(I thought this was RELA?)
2) pie emits a GOT such that there are no relocations as described in
1), because all absolute addresses are indirected by GOT (just GOT
references are relocated)
Correct. And this works really well for shared libraries, where all
text and data sections can be shared between processes, as they will
not be modified by the loader. All locations targeted by relocations
will be nicely lumped together in the GOT.

However, for bare metal style programs, there is no sharing, and there
is no advantage to lumping anything together. It is much better to use
relative references where possible, and simply apply relocations
wherever needed across the text and data sections,

If I understood the process right, but the term (GOT) is wrong, sorry,
that is what I gathered from the docs. :)
I have a x86 + PE background, so ARM + ELF is a bit of a learning curve...
The GOT is a special data structure used for implicit variable
accesses, i.e., global vars used in the code. Statically initialized
pointer variables are the other category, which are not code, and for
which the same considerations do not apply, given that the right value
simply needs to be stored in the variable before the program starts.

For instance, a statically initialized pointer always carries an
absolute address, and so it always needs an entry in the RELA table

E.g.,

int foo = 10; // external linkage
static int *bar = &foo;

In this case, there is no way to use relative addressing because the
address of foo is taken at build time.

However, if bar would be something like

static int *bar() { return &foo; }

the address is only taken at runtime, and the compiler can use a
relative reference instead, and no RELA entry is needed. With a GOT,
we force the compiler to allocate a variable that holds the absolute
address, which we would prefer to avoid.
And this is not forced by whatever table -fpie uses, as per my
understanding above?
The selection of 'code model' as it is called is controlled by GCC's
-mcmodel= argument, which defaults to 'small' on AArch64, regardless
of whether you use PIC/PIE or not.
Aha, makes sense, thanks!

Best regards,
Marvin

“Now, StandaloneMmPkg has similar (self-)relocation code too:https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c#L379-L386

Because I cannot find such elsewhere, I assume it must be for the same ARM virtualised environment as above.
No.

The binary it applies the Relocations to is documented to be the Standalone MM core, but in fact SecCore is located:

https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c#L131-L158
As per your comments below, I think SecCore should not be located here.
Is the Standalone MM core of *type* SecCore in the FFS (without *being*
SecCore)? This confused me the most.
If the FFS SecCore section type is used here, it does not mean that
the image is a SEC image in the strict PI sense.

Perhaps we were just too lazy to add a new type to the FFS spec?
That is what I meant to imply with the middle question (well, not
necessarily "lazy", for ARM there simply seems to not be any reason to
distinguish if the environments are fully separate), just wanted to make
sure I understand what the code does before modifying it.

Thank you again!

Best regards,
Marvin

“This yields the following questions to me:

1) What even invokes Standalone MM on ARM? It is documented it is spawned during SEC, but I could not find any actual invocation.
It is not spawned by the normal world code that runs UEFI. It is a
secure world component that runs in a completely different execution
context (TrustZone). The code does run with the MMU enabled from the
start, but running from an a priori fixed offset was considered to be
a security hazard, so we added self relocation support.

The alternative would have been to add metadata to the StMmCore
component that can be interpreted by the secure world component that
loads it, but this would go beyond any existing specs, and make
portability more problematic.

2) Why does Standalone MM (self-)relocation locate SecCore? Should it not already have been relocated with the code from ArmPlatformPkg? Is Standalone MM embedded into ARM SecCore?
No and no. Standalone MM has nothing to do with the code that runs as
part of UEFI itself. ArmPlatformPkg is completely separate from
StandaloneMmPkg.

3) Why is SecCore the only module relocated? Are all others guaranteed to be "properly" loaded?
SecCore contains a PE/COFF loader, so all subsequent modules are
loaded normally. This is similar to the ArmVirtQemuKernel
self-relocating SEC module, which only relocates itself in this
manner, and relies on standard PE/COFF metadata for loading other
modules.
Interesting... this definitely is vastly different from the x86 side of
things. I think most things became very clear. Thanks a lot!

4) Is there maybe some high-level documented about the ARM boot flow? It seems to be significantly different from the x86 routes quite vastly.”
trustedfirmware.org may have some useful documentation.
I'll check it some time, hopefully this weekend. Thanks!
My pleasure.


Event: TianoCore Design Meeting - APAC/NAMO - 08/06/2021 #cal-reminder

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

Reminder: TianoCore Design Meeting - APAC/NAMO

When:
08/06/2021
9:30am to 10:30am
(UTC+08:00) Asia/Shanghai

Where:
Microsoft Teams

Organizer: Ray Ni ray.ni@...

View Event

Description:

TOPIC

  1. NA

For more info, see here: https://www.tianocore.org/design-meeting/


Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 715 416 0

Alternate VTC dialing instructions

Learn More | Meeting options


Re: ArmVirt and Self-Updating Code

Marvin Häuser
 

On 23.07.21 16:34, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 16:27, Marvin Häuser <mhaeuser@posteo.de> wrote:


On 23.07.21 16:09, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 12:47, Marvin Häuser <mhaeuser@posteo.de> wrote:
On 23.07.21 12:13, Ard Biesheuvel wrote:
On Fri, 23 Jul 2021 at 11:55, Marvin Häuser <mhaeuser@posteo.de> wrote:
...
2) emit a GOT, which ends up being converted to PE/COFF Relocations (->
self-relocation), for global data that cannot be referenced relatively?
Is there any way to know/force that no symbol in GOT is accessed up
until the end of the self-relocation process?
Do you maybe have one final comment regarding that second question,
please? :)
The RELA section is not converted into PE/COFF relocations. This would
not achieve a lot, given that no prior PE/COFF loader exists to
process them. There is a snippet of asm code in the startup code that
processes the R_AARCH64_RELATIVE relocation entries before calling
into C code.
I searched for said ASM code till my fingers fell asleep and at last found this: https://github.com/tianocore/edk2/commit/b16fd231f6d8124fa05a0f086840934b8709faf9#diff-3d563cc4775c7720900f4895bf619eed06291044aaa277fcc57eddc7618351a1L12-R148

If I understand the commit message correctly, it is basically "pray the C code does not use globals at all", which is fair enough, so maybe I should document this in my proposed new library? I trust that this is enough of a constraint for both ARM and AArch64, because I do not know them at all.

What worries me is that StandaloneMmCore has no such ASM entry point at all and instead it's just executing C directly. Also, it is not passed the "-fno-jump-tables" flag that is commented to be important in the commit linked above.

Best regards,
Marvin

This also gives us the guarantee that no GOT indirections are
dereferenced, given that our asm code simply does not do that.

Let's drop "GOT" and make it "any instruction that requires prior
relocation to function correctly".
The thing to keep in mind here is that R_AARCH64_RELATIVE relocations
never target instructions, but only memory locations that carry
absolute addresses. This could be locations in .rodata or .data
(global vars carrying pointer values), or GOT entries.

It is not really a GOT. Actually, a GOT is undesirable, as it forces
global variables to be referenced via an absolute address, even when a
relative reference could be used.
Hmm, the GCC docs say a GOT is used for "all constant addresses" (I took
it as "absolute"?), it is kind of vague. I understood it this way:
1) no-pie emits relocations that can target the .text and .data sections
for instructions that embed and variables that hold an absolute address
(I thought this was RELA?)
2) pie emits a GOT such that there are no relocations as described in
1), because all absolute addresses are indirected by GOT (just GOT
references are relocated)
Correct. And this works really well for shared libraries, where all
text and data sections can be shared between processes, as they will
not be modified by the loader. All locations targeted by relocations
will be nicely lumped together in the GOT.

However, for bare metal style programs, there is no sharing, and there
is no advantage to lumping anything together. It is much better to use
relative references where possible, and simply apply relocations
wherever needed across the text and data sections,

If I understood the process right, but the term (GOT) is wrong, sorry,
that is what I gathered from the docs. :)
I have a x86 + PE background, so ARM + ELF is a bit of a learning curve...
The GOT is a special data structure used for implicit variable
accesses, i.e., global vars used in the code. Statically initialized
pointer variables are the other category, which are not code, and for
which the same considerations do not apply, given that the right value
simply needs to be stored in the variable before the program starts.

For instance, a statically initialized pointer always carries an
absolute address, and so it always needs an entry in the RELA table

E.g.,

int foo = 10; // external linkage
static int *bar = &foo;

In this case, there is no way to use relative addressing because the
address of foo is taken at build time.

However, if bar would be something like

static int *bar() { return &foo; }

the address is only taken at runtime, and the compiler can use a
relative reference instead, and no RELA entry is needed. With a GOT,
we force the compiler to allocate a variable that holds the absolute
address, which we would prefer to avoid.
And this is not forced by whatever table -fpie uses, as per my
understanding above?
The selection of 'code model' as it is called is controlled by GCC's
-mcmodel= argument, which defaults to 'small' on AArch64, regardless
of whether you use PIC/PIE or not.
Aha, makes sense, thanks!

Best regards,
Marvin

“Now, StandaloneMmPkg has similar (self-)relocation code too:https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c#L379-L386

Because I cannot find such elsewhere, I assume it must be for the same ARM virtualised environment as above.
No.

The binary it applies the Relocations to is documented to be the Standalone MM core, but in fact SecCore is located:

https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c#L131-L158
As per your comments below, I think SecCore should not be located here.
Is the Standalone MM core of *type* SecCore in the FFS (without *being*
SecCore)? This confused me the most.
If the FFS SecCore section type is used here, it does not mean that
the image is a SEC image in the strict PI sense.

Perhaps we were just too lazy to add a new type to the FFS spec?
That is what I meant to imply with the middle question (well, not
necessarily "lazy", for ARM there simply seems to not be any reason to
distinguish if the environments are fully separate), just wanted to make
sure I understand what the code does before modifying it.

Thank you again!

Best regards,
Marvin

“This yields the following questions to me:

1) What even invokes Standalone MM on ARM? It is documented it is spawned during SEC, but I could not find any actual invocation.
It is not spawned by the normal world code that runs UEFI. It is a
secure world component that runs in a completely different execution
context (TrustZone). The code does run with the MMU enabled from the
start, but running from an a priori fixed offset was considered to be
a security hazard, so we added self relocation support.

The alternative would have been to add metadata to the StMmCore
component that can be interpreted by the secure world component that
loads it, but this would go beyond any existing specs, and make
portability more problematic.

2) Why does Standalone MM (self-)relocation locate SecCore? Should it not already have been relocated with the code from ArmPlatformPkg? Is Standalone MM embedded into ARM SecCore?
No and no. Standalone MM has nothing to do with the code that runs as
part of UEFI itself. ArmPlatformPkg is completely separate from
StandaloneMmPkg.

3) Why is SecCore the only module relocated? Are all others guaranteed to be "properly" loaded?
SecCore contains a PE/COFF loader, so all subsequent modules are
loaded normally. This is similar to the ArmVirtQemuKernel
self-relocating SEC module, which only relocates itself in this
manner, and relies on standard PE/COFF metadata for loading other
modules.
Interesting... this definitely is vastly different from the x86 side of
things. I think most things became very clear. Thanks a lot!

4) Is there maybe some high-level documented about the ARM boot flow? It seems to be significantly different from the x86 routes quite vastly.”
trustedfirmware.org may have some useful documentation.
I'll check it some time, hopefully this weekend. Thanks!
My pleasure.


SLDP: Usage of PE library context by debugger?

Marvin Häuser
 

Good day everyone,

While refining the port of SourceLevelDebugPkg to my newly proposed PeCoffLib rework (RFC upcoming), I noticed that the address of the PE Image context is written to DR2 [1]. Because the UDK and Intel System Studio debugging tools are closed source, I cannot verify what happens to this value. Does the host read the library context and retrieve data from it? If not, why is its address written to DR2? If so, this would mean the new PeCoffLib implementation breaks the existing debugging tools. The following questions would arise:

1) Which data are retrieved from the context structure? For GDB, I think only the Image address and symbol file path are required (to load the symbols), while PDB is saved in DR1 already.
2) Are there any plans to provide detailed documentation of the host/client communication protocol?
3) Are there any plans to provide an open source debugger, or at least the EDK II communication protocol portion?

Thank you for your time!

Best regards,
Marvin


[1] https://github.com/tianocore/edk2/blob/610bcc69ed3d1e8c016332a1862465d41d95dd6c/SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLib.c#L126


Re: [RFC PATCH v5 07/28] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase

Erdem Aktas
 

On Wed, Jun 30, 2021 at 5:54 AM Brijesh Singh <brijesh.singh@amd.com> wrote:

a) Enhance the OVMF reset vector code to validate the pages as described
above (go through step 2 - 3).
OR
b) Validate the pages during the guest creation time. The SEV firmware
provides a command which can be used by the VMM to validate the pages
without affecting the measurement of the launch.
Are you referring to the PAGE_TYPE_UNMEASURED? Does it not affect the
measurement , PAGE_INFO will be still measured, right?

Approach #b seems much simpler; it does not require any changes to the
OVMF reset vector code.
I am worried about verifying the measurement. I understand the secret
page and cpuid page being part of measurement because both of them are
mentioned in the AMD SNP SPEC but now we are introducing a new
parameters (all the 4KB page addresses between SNP_HV_VALIDATED_START
and SNP_HV_VALIDATED_END) that VM owner needs to know to calculate the
measurement and verify the attestation.

Sorry if I am overthinking or missing something here.

-Erdem


Re: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

Yao, Jiewen
 

I agree with Bret. Adding EmbeddedPkg dependency is not a good idea.

 

Another option: Is there any possibility to move the function to EmbeddedPkg and just add a simple function call in StanaloneMmPkg ?

 

Thank you

Yao Jiewen

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Andrew Fish via groups.io
Sent: Saturday, July 31, 2021 8:45 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; bret.barkelew@...
Cc: sayanta.pattanayak@...; Ard Biesheuvel <ardb+tianocore@...>; Sami Mujawar <sami.mujawar@...>
Subject: Re: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

 

 



On Jul 30, 2021, at 12:34 PM, Bret Barkelew via groups.io <bret.barkelew@...> wrote:

 

I don’t think this is a good dependency. StandaloneMmCore shouldn’t require anything from EmbeddedPkg, and if it does there’s probably some more generalization necessary.

 

Further, we can’t require the MmCore (which should be considered generic at the level of PEI Core or DXE Core) include some external library like Libfdt without significant justification.

 

 

I think we should understand if this is being driven by some kind of standard? Maybe the Flat Device Tree lib should be part of the MdePkg? Or maybe MdeModulePkg?

 

Thanks,

 

Andrew Fish



- Bret 

 

From: Sayanta Pattanayak via groups.io
Sent: Friday, July 30, 2021 10:36 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel; Sami Mujawar; Achin Gupta
Subject: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

 

Introduce support to populate StMM boot data via DTS parsing. The DTB is
passed as a boot argument by a binary of higer exception level.
Previously it was achieved by placing the boot data structure in a
shared buffer and the address of this shared buffer was passed by the
binary of higher exception level. Now either of the option can be used
for populating StMM boot info.

StMM boot information structure binding in device tree can be of following
prototype. Property values are not mentioned here.

bootarg {
  compatible = "bootargs";
  h_type  = <..>;
  h_version = <..>;
  h_size    = <..>;
  h_attr    = <..>;
  sp_mem_base         = <..>;
  sp_mem_limit        = <..>;
  sp_image_base       = <..>;
  sp_stack_base       = <..>;
  sp_heap_base        = <..>;
  sp_ns_comm_buf_base = <..>;fc
  sp_shared_buf_base  = <..>;
  sp_image_size       = <..>;
  sp_pcpu_stack_size  = <..>;
  sp_heap_size        = <..>;
  sp_ns_comm_buf_size = <..>;
  sp_shared_buf_size  = <..>;
  num_sp_mem_regions  = <..>;
  num_cpus            = <..>;
};

Addition of DTS supoort involves a dependency on FdtLib from EmbeddedPkg.

Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@...>
---
 Link to github branch with this patch -
 https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSayantaP-arm%2Fedk2%2Ftree%2Fstmm-dts&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gJw%2BH8Dj9lih1DNrIzbNPToJn9nH%2BHqpvpSr2JJrS4I%3D&amp;reserved=0

 StandaloneMmPkg/StandaloneMmPkg.dsc                                                     |   1 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |   3 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 153 ++++++++++++++++++--
 3 files changed, 143 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 0c45df95e2dd..e3a3a6ee3ba1 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -49,6 +49,7 @@
   HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
   MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
   MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
   MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58ef4..0a2e519dd664 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -30,6 +30,7 @@
   X64/StandaloneMmCoreEntryPoint.c
 
 [Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   StandaloneMmPkg/StandaloneMmPkg.dec
@@ -40,10 +41,12 @@
 [LibraryClasses]
   BaseLib
   DebugLib
+  FdtLib
 
 [LibraryClasses.AARCH64]
   StandaloneMmMmuLib
   ArmSvcLib
+  FdtLib
 
 [Guids]
   gMpInformationHobGuid
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 6c50f470aa35..cc09d75dac36 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/MmramMemoryReserve.h>
 #include <Guid/MpInformation.h>
 
+#include <libfdt.h>
 #include <Library/ArmMmuLib.h>
 #include <Library/ArmSvcLib.h>
 #include <Library/DebugLib.h>
@@ -45,33 +46,31 @@ STATIC CONST UINT32 mSpmMinorVerFfa = SPM_MINOR_VERSION_FFA;
 PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT      CpuDriverEntryPoint = NULL;
 
 /**
-  Retrieve a pointer to and print the boot information passed by privileged
-  secure firmware.
+  Prints boot information.
 
-  @param  [in] SharedBufAddress   The pointer memory shared with privileged
-                                  firmware.
+  This function prints the boot information, which is passed by privileged
+  secure firmware through shared buffer or other mechanism.
 
+  @param  [in] PayloadBootInfo   Pointer to StandaloneMM Boot Info structure.
 **/
-EFI_SECURE_PARTITION_BOOT_INFO *
-GetAndPrintBootinformation (
-  IN VOID                      *SharedBufAddress
+VOID
+PrintBootinformation (
+  IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo
 )
 {
-  EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
   EFI_SECURE_PARTITION_CPU_INFO  *PayloadCpuInfo;
   UINTN                          Index;
 
-  PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *) SharedBufAddress;
 
   if (PayloadBootInfo == NULL) {
     DEBUG ((DEBUG_ERROR, "PayloadBootInfo NULL\n"));
-    return NULL;
+    return;
   }
 
   if (PayloadBootInfo->Header.Version != BOOT_PAYLOAD_VERSION) {
     DEBUG ((DEBUG_ERROR, "Boot Information Version Mismatch. Current=0x%x, Expected=0x%x.\n",
             PayloadBootInfo->Header.Version, BOOT_PAYLOAD_VERSION));
-    return NULL;
+    return;
   }
 
   DEBUG ((DEBUG_INFO, "NumSpMemRegions - 0x%x\n", PayloadBootInfo->NumSpMemRegions));
@@ -96,7 +95,7 @@ GetAndPrintBootinformation (
 
   if (PayloadCpuInfo == NULL) {
     DEBUG ((DEBUG_ERROR, "PayloadCpuInfo NULL\n"));
-    return NULL;
+    return;
   }
 
   for (Index = 0; Index < PayloadBootInfo->NumCpus; Index++) {
@@ -105,7 +104,7 @@ GetAndPrintBootinformation (
     DEBUG ((DEBUG_INFO, "Flags           - 0x%x\n", PayloadCpuInfo[Index].Flags));
   }
 
-  return PayloadBootInfo;
+  return;
 }
 
 /**
@@ -194,6 +193,119 @@ DelegatedEventLoop (
   }
 }
 
+/**
+  Populates StandAloneMM boot information structure.
+
+  This function receives dtb Address, where StMM Boot information specific
+  properties will be looked out to form the booting structure of type
+  EFI_SECURE_PARTITION_BOOT_INFO. At first, the properties for StandAloneMM
+  ConfigSize and  Memory limit will be checked out. Boot information will
+  be stored at address (Memory Limit - ConfigSize). Thereafter all boot
+  information specific properties will be parsed and corresponding values
+  will be obtained.
+
+  @param  [out] BootInfo   Pointer, where Boot Info structure will be populated.
+  @param  [in] DtbAddress  Address of the Device tree from where Boot
+                           information will be fetched.
+**/
+VOID
+PopulateBootinformation (
+  OUT EFI_SECURE_PARTITION_BOOT_INFO **BootInfo,
+  IN VOID   *DtbAddress
+)
+{
+  INT32           Offset;
+  CONST UINT32    *Property;
+  CONST UINT64    *Property64;
+  UINT32          ConfigSize;
+  UINT64          SpMemLimit;
+  EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
+
+  Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "config-size");
+  if (Offset < 0) {
+    DEBUG ((DEBUG_WARN, "Total Config Size is not  defined\n"));
+  } else {
+    Property = fdt_getprop (DtbAddress, Offset, "size", NULL);
+    if (Property) {
+      ConfigSize = fdt32_to_cpu (*Property);
+      DEBUG ((DEBUG_INFO, "stmm dtb config-size  = 0x%x \n", ConfigSize));
+    }
+  }
+
+  Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "bootargs");
+  if (Offset >= 0) {
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_mem_limit", NULL);
+    SpMemLimit = fdt64_to_cpu (*Property64);
+  }
+
+  if (SpMemLimit && ConfigSize)
+    PayloadBootInfo =
+      (EFI_SECURE_PARTITION_BOOT_INFO *)(SpMemLimit - ConfigSize);
+
+  if (PayloadBootInfo) {
+    PayloadBootInfo->SpMemLimit = SpMemLimit;
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_type", NULL);
+    PayloadBootInfo->Header.Type = (UINT8) fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_version", NULL);
+    PayloadBootInfo->Header.Version = (UINT8) fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_size", NULL);
+    PayloadBootInfo->Header.Size = (UINT8) fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_attr", NULL);
+    PayloadBootInfo->Header.Attr = fdt32_to_cpu(*Property);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_mem_base", NULL);
+    PayloadBootInfo->SpMemBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_image_base", NULL);
+    PayloadBootInfo->SpImageBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_stack_base", NULL);
+    PayloadBootInfo->SpStackBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_heap_base", NULL);
+    PayloadBootInfo->SpHeapBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_base", NULL);
+    PayloadBootInfo->SpNsCommBufBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_shared_buf_base", NULL);
+    PayloadBootInfo->SpSharedBufBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_image_size", NULL);
+    PayloadBootInfo->SpImageSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_pcpu_stack_size", NULL);
+    PayloadBootInfo->SpPcpuStackSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_heap_size", NULL);
+    PayloadBootInfo->SpHeapSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_size", NULL);
+    PayloadBootInfo->SpNsCommBufSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_shared_buf_size", NULL);
+    PayloadBootInfo->SpPcpuSharedBufSize = fdt64_to_cpu(*Property64);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "num_sp_mem_regions", NULL);
+    PayloadBootInfo->NumSpMemRegions = fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "num_cpus", NULL);
+    PayloadBootInfo->NumCpus = fdt32_to_cpu(*Property);
+
+    PayloadBootInfo->CpuInfo =
+      (EFI_SECURE_PARTITION_CPU_INFO *)((UINT64)PayloadBootInfo +
+                                        sizeof(EFI_SECURE_PARTITION_BOOT_INFO));
+  }
+
+  *BootInfo = PayloadBootInfo;
+
+  return;
+}
+
 /**
   Query the SPM version, check compatibility and return success if compatible.
 
@@ -313,6 +425,7 @@ _ModuleEntryPoint (
   VOID                                    *TeData;
   UINTN                                   TeDataSize;
   EFI_PHYSICAL_ADDRESS                    ImageBase;
+  VOID                                    *DtbAddress;
 
   // Get Secure Partition Manager Version Information
   Status = GetSpmVersion ();
@@ -320,12 +433,24 @@ _ModuleEntryPoint (
     goto finish;
   }
 
-  PayloadBootInfo = GetAndPrintBootinformation (SharedBufAddress);
+  // In cookie1 the DTB address is passed. With reference to DTB, Boot
+  // info structure can be populated.
+  // If cookie1 doesn't have any value, then Boot info is copied from
+  // Sharedbuffer.
+  if (cookie1) {
+    DtbAddress = (void *)cookie1;
+    PopulateBootinformation (&PayloadBootInfo, DtbAddress);
+  } else {
+    PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *)SharedBufAddress;
+  }
+
   if (PayloadBootInfo == NULL) {
     Status = EFI_UNSUPPORTED;
     goto finish;
   }
 
+  PrintBootinformation (PayloadBootInfo);
+
   // Locate PE/COFF File information for the Standalone MM core module
   Status = LocateStandaloneMmCorePeCoffData (
              (EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
-- 
2.17.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78449): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78449&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=0DbkyBJe0prgx2vSCVVsrI0GoVbCipcfmbZp83LREjo%3D&amp;reserved=0
Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F84555304%2F1852292&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=1u3cr0MmJmjrkxtGgfOe1mb8%2F7vUXaoyUr5%2FmC1Uq%2F8%3D&amp;reserved=0
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786142135%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Yr%2BSNqWHllfxHkFqBhCyP2TE4IGRLFLm%2FCCWexjB0AM%3D&amp;reserved=0 [bret.barkelew@...]
-=-=-=-=-=-=


 

 


Re: [RFC] MemoryProtectionLib for Dynamic Memory Guard Settings

Yao, Jiewen
 

Hi Sean
That is excellent topic.

I agree with you that, one of most frequent asked question in the design meeting is: Should I use protocol or library? Sigh...

I added my personal thought below. I would like other people to comment, Mike Kinney, Andrew Fish, Liming Gao, who knows the history and EDKII design philosophy very well.

==========
Besides the specification difference you mentioned in previous email,

1) *Protocol* is between 2 modules in UEFI/SMM phase. It is used to pass data or abstract function.
2) *PPI* is similar to *Protocol*. But it is between 2 modules in PEI phase.

3) *HOB* is between 2 modules in different phase, to pass the data from one phase to the other. Such as PEI passing HOB to DXE, PEI passing HOB to SMM.
4) *Variable* is also between 2 modules in different phase. In addition, the data can be *NON_VOLATILE* and always be there across boot.
5) *GUID configuration table* is also between 2 modules in different phase, but it is limited from UEFI to OS. From function perspective, GUID HOB is same as GUID Config Table. The difference is that HOB is PI/OEM Firmware scope, while the GUID Config Table is UEFI/OS interface scope.

So far, above is the EDK-I design. However, two big issues existed in EDK-I.
A) The policy protocol. Everybody hated the policy protocol in EDKII, but everybody has to use it because there is no other choice. Everyone keeps increase the policy version and adds their own stuff, which make code hard to unify.
B) The library. In EDK-I, we had crazy library design - EfiDriverLib, EfiCommonLib, EfiRuntimeLib, EfiShellLib. It looked good at first glance. But when we link them together, it fails. 2 libraries may have same function name, but implement in different way. 2 libraries may have different function name, but implement same function.

Those issues drove the two most important feature - PCD and Library design in EDK-II.

6) *PCD* is designed to abstract the OEM configuration data.
Static PCD means OEM can determine the value at build time.
Dynamic PCD means OEM need determine the value at runtime. (The implementation of PCD is still based upon PPI, Hob, and Protocol.)

7) A generic *library* is design to share common code in one place.
In addition, EDKII *library*serves another purpose: to abstract a set of common interface (as known as Library Class). That allows the OEM can replace the library implementation (as known as Library Instance) at build time without changing any code.

==========

Usually, we expose functions in a library instead of global data. (Well, there are exceptions such as gBS, gST, gRT, gSmst, which is well known pointers in history.)
I did not see too many cases to expose global data directly.

To compare UEFI protocol or C structure/class, I feel option 1 is similar to :
typedef struct {
Public:
UINTN Data;
} CLASS_XXX.

While, option 2 is similar to :
typedef struct {
Private:
UINTN Data;
Public:
UINTN GetData ();
VOID SetData (UINTN Data);
} CLASS_XXX.


I also see the similarity with https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/DebugLib.h, such as DebugAssertEnabled(), DebugPrintEnabled(), DebugCodeEnabled(), DebugClearMemoryEnabled(), DebugPrintLevelEnabled().
That is the reason I vote option 2.


Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
Sent: Saturday, July 31, 2021 2:42 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Taylor Beebe
<t@taylorbeebe.com>; Wang, Jian J <jian.j.wang@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
Rahul1 <rahul1.kumar@intel.com>; mikuback@linux.microsoft.com; Wu, Hao A
<hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
gaoliming@byosoft.com.cn; Dong, Guo <guo.dong@intel.com>; Ma, Maurice
<maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>
Subject: Re: [edk2-devel] [RFC] MemoryProtectionLib for Dynamic Memory
Guard Settings

Jiewen,

**Slight rant**

I agree with libraries as an effective abstraction method. But I think
there needs to be a broad discussion about the order of preference for
methods of abstraction. Today the edk2 code base is a mix and often
there are numerous methods abstracting the same thing which leads to
confusion, misconfiguration, and error.

In the UEFI specification we have PPIs/Protocols/Events for functional
abstraction. We have variables, guided config tables, and HII for data
abstraction.

In the PI specification we add HOBs and PCDs for data abstractions.

Finally, in EDKII we add the library class concept and leverage it
heavily for arch, phase, and platform/behavioral abstractions.

Without clear guidance for how and when to use the above it is hard to
keep code being developed by the larger community consistent.

**End**

I was leaning towards something closer to

>> Option 1:
https://github.com/TaylorBeebe/edk2/tree/memory_protection_lib_2

the HOB method and internally as we develop more code we are preferring
HOB and data abstractions more than functional abstraction. Data
abstractions can be used to control functional differences as well if
needed. Data abstractions allow for easier validation and support
diverse code environments. For example standalone MM and
payloadpkg/payload concepts. Finally, data abstractions break the need
for a monolithic code base. But as you can see in option 1 it actually
uses a library class abstraction as well because no one wants to write
the same code over and over again to get the HOB. The contract of the
library is just data but it still requires library mappings. Maybe
these types of libraries need to be treated differently.

Anyway it would be great to hear from other members of the community
around not just the memory protections RFC (this RFC) but around
preferences for abstraction techniques (pro/con). If an actual
discussion starts it could move to design meeting.

Thanks
Sean







On 7/29/2021 7:34 PM, Yao, Jiewen wrote:
Thanks. Code talks better.

I prefer option 2, which is a generic way for abstraction.

And you may enable option 1 under the cover of option 2, just create a lib
instance to get config from Hob.

Thank you
Yao Jiewen

-----Original Message-----
From: Taylor Beebe <t@taylorbeebe.com>
Sent: Friday, July 30, 2021 10:07 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>;
devel@edk2.groups.io
Cc: spbrogan@outlook.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>;
mikuback@linux.microsoft.com; Wu, Hao A <hao.a.wu@intel.com>; Bi,
Dandan
<dandan.bi@intel.com>; gaoliming@byosoft.com.cn; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You,
Benjamin
<benjamin.you@intel.com>
Subject: Re: [RFC] MemoryProtectionLib for Dynamic Memory Guard Settings

Of course - here are a couple of rough drafts:

Option 1:
https://github.com/TaylorBeebe/edk2/tree/memory_protection_lib_2
Option 2:
https://github.com/TaylorBeebe/edk2/tree/memory_protection_lib

On 7/29/2021 6:57 PM, Yao, Jiewen wrote:
Hi
Sorry, I am not able to follow the discussion.

Is there any sample or POC code to show the concept?

-----Original Message-----
From: Taylor Beebe <t@taylorbeebe.com>
Sent: Friday, July 30, 2021 9:55 AM
To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
Cc: spbrogan@outlook.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>;
mikuback@linux.microsoft.com; Wu, Hao A <hao.a.wu@intel.com>; Bi,
Dandan
<dandan.bi@intel.com>; gaoliming@byosoft.com.cn; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You,
Benjamin
<benjamin.you@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [RFC] MemoryProtectionLib for Dynamic Memory Guard
Settings

Thanks for your feedback, Jian.

In option 2, a most basic implementation would returning the current
FixedAtBuild PCDs assuming they are kept. If they aren't, the library
implementer could simply hard-code the return value for each memory
protection setting.

In option 1, the HOB would be published in pre-mem and I'm not an expert
on exploiting the pre-mem environment. Jiewen may have more to say on
this.

-Taylor

On 7/28/2021 7:18 PM, Wang, Jian J wrote:
Thanks for the RFC. I'm not object to this idea. The only concern from me
is the potential security holes introduced by the changes. According to
your
description, it allows 3rd party software to violate memory protection
policy.
I'd like to see more explanations on how to avoid it to be exploited.

+Jiewen, what's current process to evaluate the security threat?

Regards,
Jian

-----Original Message-----
From: Taylor Beebe <t@taylorbeebe.com>
Sent: Friday, July 23, 2021 8:33 AM
To: devel@edk2.groups.io
Cc: spbrogan@outlook.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <Rahul1.Kumar@intel.com>;
mikuback@linux.microsoft.com; Wang, Jian J <jian.j.wang@intel.com>;
Wu,
Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
gaoliming@byosoft.com.cn; Dong, Guo <guo.dong@intel.com>; Ma,
Maurice
<maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>
Subject: [RFC] MemoryProtectionLib for Dynamic Memory Guard
Settings

Current memory protection settings rely on FixedAtBuild PCD values
(minus PcdSetNxForStack). Because of this, the memory protection
configuration interface is fixed in nature. Cases arise in which memory
protections might need to be adjusted between boots (if platform design
allows) to avoid disabling a system. For example, platforms might
choose
to allow the user to control their protection policies such as allow
execution of critical 3rd party software that might violate memory
protections.

This RFC seeks your feedback regarding introducing an interface that
allows dynamic configuration of memory protection settings.

I would like to propose two options:
1. Describing the memory protection setting configuration in a HOB that
is produced by the platform.
2. Introducing a library class (e.g. MemoryProtectionLib) that allows
abstraction of the memory protection setting configuration data source.

In addition, I would like to know if the memory protection FixedAtBuild
PCDs currently in MdeModulePkg can be removed so we can move the
configuration interface entirely to an option above.

In any case, I would like the settings to be visible to environments
such as Standalone MM where dynamic PCDs are not accessible.

I am seeking your feedback on this proposal in preparation for sending
an edk2 patch series.

--
Taylor Beebe
Software Engineer @ Microsoft
--
Taylor Beebe
Software Engineer @ Microsoft
--
Taylor Beebe
Software Engineer @ Microsoft






Re: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

Andrew Fish
 



On Jul 30, 2021, at 12:34 PM, Bret Barkelew via groups.io <bret.barkelew@...> wrote:

I don’t think this is a good dependency. StandaloneMmCore shouldn’t require anything from EmbeddedPkg, and if it does there’s probably some more generalization necessary.
 
Further, we can’t require the MmCore (which should be considered generic at the level of PEI Core or DXE Core) include some external library like Libfdt without significant justification.
 

I think we should understand if this is being driven by some kind of standard? Maybe the Flat Device Tree lib should be part of the MdePkg? Or maybe MdeModulePkg?

Thanks,

Andrew Fish

- Bret 
 
From: Sayanta Pattanayak via groups.io
Sent: Friday, July 30, 2021 10:36 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel; Sami Mujawar; Achin Gupta
Subject: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree
 

Introduce support to populate StMM boot data via DTS parsing. The DTB is
passed as a boot argument by a binary of higer exception level.
Previously it was achieved by placing the boot data structure in a
shared buffer and the address of this shared buffer was passed by the
binary of higher exception level. Now either of the option can be used
for populating StMM boot info.

StMM boot information structure binding in device tree can be of following
prototype. Property values are not mentioned here.

bootarg {
  compatible = "bootargs";
  h_type  = <..>;
  h_version = <..>;
  h_size    = <..>;
  h_attr    = <..>;
  sp_mem_base         = <..>;
  sp_mem_limit        = <..>;
  sp_image_base       = <..>;
  sp_stack_base       = <..>;
  sp_heap_base        = <..>;
  sp_ns_comm_buf_base = <..>;fc
  sp_shared_buf_base  = <..>;
  sp_image_size       = <..>;
  sp_pcpu_stack_size  = <..>;
  sp_heap_size        = <..>;
  sp_ns_comm_buf_size = <..>;
  sp_shared_buf_size  = <..>;
  num_sp_mem_regions  = <..>;
  num_cpus            = <..>;
};

Addition of DTS supoort involves a dependency on FdtLib from EmbeddedPkg.

Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@...>
---
 Link to github branch with this patch -
 https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSayantaP-arm%2Fedk2%2Ftree%2Fstmm-dts&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gJw%2BH8Dj9lih1DNrIzbNPToJn9nH%2BHqpvpSr2JJrS4I%3D&amp;reserved=0

 StandaloneMmPkg/StandaloneMmPkg.dsc                                                     |   1 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |   3 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 153 ++++++++++++++++++--
 3 files changed, 143 insertions(+), 14 deletions(-)

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 0c45df95e2dd..e3a3a6ee3ba1 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -49,6 +49,7 @@
   HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
   MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
   MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
   MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58ef4..0a2e519dd664 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -30,6 +30,7 @@
   X64/StandaloneMmCoreEntryPoint.c
 
 [Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   StandaloneMmPkg/StandaloneMmPkg.dec
@@ -40,10 +41,12 @@
 [LibraryClasses]
   BaseLib
   DebugLib
+  FdtLib
 
 [LibraryClasses.AARCH64]
   StandaloneMmMmuLib
   ArmSvcLib
+  FdtLib
 
 [Guids]
   gMpInformationHobGuid
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 6c50f470aa35..cc09d75dac36 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/MmramMemoryReserve.h>
 #include <Guid/MpInformation.h>
 
+#include <libfdt.h>
 #include <Library/ArmMmuLib.h>
 #include <Library/ArmSvcLib.h>
 #include <Library/DebugLib.h>
@@ -45,33 +46,31 @@ STATIC CONST UINT32 mSpmMinorVerFfa = SPM_MINOR_VERSION_FFA;
 PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT      CpuDriverEntryPoint = NULL;
 
 /**
-  Retrieve a pointer to and print the boot information passed by privileged
-  secure firmware.
+  Prints boot information.
 
-  @param  [in] SharedBufAddress   The pointer memory shared with privileged
-                                  firmware.
+  This function prints the boot information, which is passed by privileged
+  secure firmware through shared buffer or other mechanism.
 
+  @param  [in] PayloadBootInfo   Pointer to StandaloneMM Boot Info structure.
 **/
-EFI_SECURE_PARTITION_BOOT_INFO *
-GetAndPrintBootinformation (
-  IN VOID                      *SharedBufAddress
+VOID
+PrintBootinformation (
+  IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo
 )
 {
-  EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
   EFI_SECURE_PARTITION_CPU_INFO  *PayloadCpuInfo;
   UINTN                          Index;
 
-  PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *) SharedBufAddress;
 
   if (PayloadBootInfo == NULL) {
     DEBUG ((DEBUG_ERROR, "PayloadBootInfo NULL\n"));
-    return NULL;
+    return;
   }
 
   if (PayloadBootInfo->Header.Version != BOOT_PAYLOAD_VERSION) {
     DEBUG ((DEBUG_ERROR, "Boot Information Version Mismatch. Current=0x%x, Expected=0x%x.\n",
             PayloadBootInfo->Header.Version, BOOT_PAYLOAD_VERSION));
-    return NULL;
+    return;
   }
 
   DEBUG ((DEBUG_INFO, "NumSpMemRegions - 0x%x\n", PayloadBootInfo->NumSpMemRegions));
@@ -96,7 +95,7 @@ GetAndPrintBootinformation (
 
   if (PayloadCpuInfo == NULL) {
     DEBUG ((DEBUG_ERROR, "PayloadCpuInfo NULL\n"));
-    return NULL;
+    return;
   }
 
   for (Index = 0; Index < PayloadBootInfo->NumCpus; Index++) {
@@ -105,7 +104,7 @@ GetAndPrintBootinformation (
     DEBUG ((DEBUG_INFO, "Flags           - 0x%x\n", PayloadCpuInfo[Index].Flags));
   }
 
-  return PayloadBootInfo;
+  return;
 }
 
 /**
@@ -194,6 +193,119 @@ DelegatedEventLoop (
   }
 }
 
+/**
+  Populates StandAloneMM boot information structure.
+
+  This function receives dtb Address, where StMM Boot information specific
+  properties will be looked out to form the booting structure of type
+  EFI_SECURE_PARTITION_BOOT_INFO. At first, the properties for StandAloneMM
+  ConfigSize and  Memory limit will be checked out. Boot information will
+  be stored at address (Memory Limit - ConfigSize). Thereafter all boot
+  information specific properties will be parsed and corresponding values
+  will be obtained.
+
+  @param  [out] BootInfo   Pointer, where Boot Info structure will be populated.
+  @param  [in] DtbAddress  Address of the Device tree from where Boot
+                           information will be fetched.
+**/
+VOID
+PopulateBootinformation (
+  OUT EFI_SECURE_PARTITION_BOOT_INFO **BootInfo,
+  IN VOID   *DtbAddress
+)
+{
+  INT32           Offset;
+  CONST UINT32    *Property;
+  CONST UINT64    *Property64;
+  UINT32          ConfigSize;
+  UINT64          SpMemLimit;
+  EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
+
+  Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "config-size");
+  if (Offset < 0) {
+    DEBUG ((DEBUG_WARN, "Total Config Size is not  defined\n"));
+  } else {
+    Property = fdt_getprop (DtbAddress, Offset, "size", NULL);
+    if (Property) {
+      ConfigSize = fdt32_to_cpu (*Property);
+      DEBUG ((DEBUG_INFO, "stmm dtb config-size  = 0x%x \n", ConfigSize));
+    }
+  }
+
+  Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "bootargs");
+  if (Offset >= 0) {
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_mem_limit", NULL);
+    SpMemLimit = fdt64_to_cpu (*Property64);
+  }
+
+  if (SpMemLimit && ConfigSize)
+    PayloadBootInfo =
+      (EFI_SECURE_PARTITION_BOOT_INFO *)(SpMemLimit - ConfigSize);
+
+  if (PayloadBootInfo) {
+    PayloadBootInfo->SpMemLimit = SpMemLimit;
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_type", NULL);
+    PayloadBootInfo->Header.Type = (UINT8) fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_version", NULL);
+    PayloadBootInfo->Header.Version = (UINT8) fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_size", NULL);
+    PayloadBootInfo->Header.Size = (UINT8) fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "h_attr", NULL);
+    PayloadBootInfo->Header.Attr = fdt32_to_cpu(*Property);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_mem_base", NULL);
+    PayloadBootInfo->SpMemBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_image_base", NULL);
+    PayloadBootInfo->SpImageBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_stack_base", NULL);
+    PayloadBootInfo->SpStackBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_heap_base", NULL);
+    PayloadBootInfo->SpHeapBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_base", NULL);
+    PayloadBootInfo->SpNsCommBufBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_shared_buf_base", NULL);
+    PayloadBootInfo->SpSharedBufBase = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_image_size", NULL);
+    PayloadBootInfo->SpImageSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_pcpu_stack_size", NULL);
+    PayloadBootInfo->SpPcpuStackSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_heap_size", NULL);
+    PayloadBootInfo->SpHeapSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_size", NULL);
+    PayloadBootInfo->SpNsCommBufSize = fdt64_to_cpu(*Property64);
+
+    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_shared_buf_size", NULL);
+    PayloadBootInfo->SpPcpuSharedBufSize = fdt64_to_cpu(*Property64);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "num_sp_mem_regions", NULL);
+    PayloadBootInfo->NumSpMemRegions = fdt32_to_cpu(*Property);
+
+    Property =  fdt_getprop (DtbAddress, Offset, "num_cpus", NULL);
+    PayloadBootInfo->NumCpus = fdt32_to_cpu(*Property);
+
+    PayloadBootInfo->CpuInfo =
+      (EFI_SECURE_PARTITION_CPU_INFO *)((UINT64)PayloadBootInfo +
+                                        sizeof(EFI_SECURE_PARTITION_BOOT_INFO));
+  }
+
+  *BootInfo = PayloadBootInfo;
+
+  return;
+}
+
 /**
   Query the SPM version, check compatibility and return success if compatible.
 
@@ -313,6 +425,7 @@ _ModuleEntryPoint (
   VOID                                    *TeData;
   UINTN                                   TeDataSize;
   EFI_PHYSICAL_ADDRESS                    ImageBase;
+  VOID                                    *DtbAddress;
 
   // Get Secure Partition Manager Version Information
   Status = GetSpmVersion ();
@@ -320,12 +433,24 @@ _ModuleEntryPoint (
     goto finish;
   }
 
-  PayloadBootInfo = GetAndPrintBootinformation (SharedBufAddress);
+  // In cookie1 the DTB address is passed. With reference to DTB, Boot
+  // info structure can be populated.
+  // If cookie1 doesn't have any value, then Boot info is copied from
+  // Sharedbuffer.
+  if (cookie1) {
+    DtbAddress = (void *)cookie1;
+    PopulateBootinformation (&PayloadBootInfo, DtbAddress);
+  } else {
+    PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *)SharedBufAddress;
+  }
+
   if (PayloadBootInfo == NULL) {
     Status = EFI_UNSUPPORTED;
     goto finish;
   }
 
+  PrintBootinformation (PayloadBootInfo);
+
   // Locate PE/COFF File information for the Standalone MM core module
   Status = LocateStandaloneMmCorePeCoffData (
              (EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
-- 
2.17.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78449): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78449&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=0DbkyBJe0prgx2vSCVVsrI0GoVbCipcfmbZp83LREjo%3D&amp;reserved=0
Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F84555304%2F1852292&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=1u3cr0MmJmjrkxtGgfOe1mb8%2F7vUXaoyUr5%2FmC1Uq%2F8%3D&amp;reserved=0
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786142135%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Yr%2BSNqWHllfxHkFqBhCyP2TE4IGRLFLm%2FCCWexjB0AM%3D&amp;reserved=0 [bret.barkelew@...]
-=-=-=-=-=-=

 


Re: [RFC] MemoryProtectionLib for Dynamic Memory Guard Settings

Taylor Beebe
 

I am going to extend the opportunity for feedback to the end of Tuesday. I would very much like more community input on this proposal.

On 7/30/2021 11:42 AM, Sean Brogan wrote:
Jiewen,
**Slight rant**
I agree with libraries as an effective abstraction method.  But I think there needs to be a broad discussion about the order of preference for methods of abstraction.  Today the edk2 code base is a mix and often there are numerous methods abstracting the same thing which leads to confusion, misconfiguration, and error.
In the UEFI specification we have PPIs/Protocols/Events for functional abstraction.  We have variables, guided config tables, and HII for data abstraction.
In the PI specification we add HOBs and PCDs for data abstractions.
Finally, in EDKII we add the library class concept and leverage it heavily for arch, phase, and platform/behavioral abstractions.
Without clear guidance for how and when to use the above it is hard to keep code being developed by the larger community consistent.
**End**
I was leaning towards something closer to

>> Option 1:
https://github.com/TaylorBeebe/edk2/tree/memory_protection_lib_2
the HOB method and internally as we develop more code we are preferring HOB and data abstractions more than functional abstraction.  Data abstractions can be used to control functional differences as well if needed.  Data abstractions allow for easier validation and support diverse code environments.  For example standalone MM and payloadpkg/payload concepts.  Finally, data abstractions break the need for a monolithic code base.   But as you can see in option 1 it actually uses a library class abstraction as well because no one wants to write the same code over and over again to get the HOB.  The contract of the library is just data but it still requires library mappings.  Maybe these types of libraries need to be treated differently.
Anyway it would be great to hear from other members of the community around not just the memory protections RFC (this RFC) but around preferences for abstraction techniques (pro/con).  If an actual discussion starts it could move to design meeting.
Thanks
Sean
On 7/29/2021 7:34 PM, Yao, Jiewen wrote:
Thanks. Code talks better.

I prefer option 2, which is a generic way for abstraction.

And you may enable option 1 under the cover of option 2, just create a lib instance to get config from Hob.

Thank you
Yao Jiewen

-----Original Message-----
From: Taylor Beebe <t@taylorbeebe.com>
Sent: Friday, July 30, 2021 10:07 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
devel@edk2.groups.io
Cc: spbrogan@outlook.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>;
mikuback@linux.microsoft.com; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; gaoliming@byosoft.com.cn; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
<benjamin.you@intel.com>
Subject: Re: [RFC] MemoryProtectionLib for Dynamic Memory Guard Settings

Of course - here are a couple of rough drafts:

Option 1: https://github.com/TaylorBeebe/edk2/tree/memory_protection_lib_2
Option 2: https://github.com/TaylorBeebe/edk2/tree/memory_protection_lib

On 7/29/2021 6:57 PM, Yao, Jiewen wrote:
Hi
Sorry, I am not able to follow the discussion.

Is there any sample or POC code to show the concept?

-----Original Message-----
From: Taylor Beebe <t@taylorbeebe.com>
Sent: Friday, July 30, 2021 9:55 AM
To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
Cc: spbrogan@outlook.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>;
mikuback@linux.microsoft.com; Wu, Hao A <hao.a.wu@intel.com>; Bi,
Dandan
<dandan.bi@intel.com>; gaoliming@byosoft.com.cn; Dong, Guo
<guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You,
Benjamin
<benjamin.you@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [RFC] MemoryProtectionLib for Dynamic Memory Guard Settings

Thanks for your feedback, Jian.

In option 2, a most basic implementation would returning the current
FixedAtBuild PCDs assuming they are kept. If they aren't, the library
implementer could simply hard-code the return value for each memory
protection setting.

In option 1, the HOB would be published in pre-mem and I'm not an expert
on exploiting the pre-mem environment. Jiewen may have more to say on
this.

-Taylor

On 7/28/2021 7:18 PM, Wang, Jian J wrote:
Thanks for the RFC. I'm not object to this idea. The only concern from me
is the potential security holes introduced by the changes. According to your
description, it allows 3rd party software to violate memory protection
policy.
I'd like to see more explanations on how to avoid it to be exploited.

+Jiewen, what's current process to evaluate the security threat?

Regards,
Jian

-----Original Message-----
From: Taylor Beebe <t@taylorbeebe.com>
Sent: Friday, July 23, 2021 8:33 AM
To: devel@edk2.groups.io
Cc: spbrogan@outlook.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <Rahul1.Kumar@intel.com>;
mikuback@linux.microsoft.com; Wang, Jian J <jian.j.wang@intel.com>;
Wu,
Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
gaoliming@byosoft.com.cn; Dong, Guo <guo.dong@intel.com>; Ma,
Maurice
<maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>
Subject: [RFC] MemoryProtectionLib for Dynamic Memory Guard Settings

Current memory protection settings rely on FixedAtBuild PCD values
(minus PcdSetNxForStack). Because of this, the memory protection
configuration interface is fixed in nature. Cases arise in which memory
protections might need to be adjusted between boots (if platform design
allows) to avoid disabling a system. For example, platforms might choose
to allow the user to control their protection policies such as allow
execution of critical 3rd party software that might violate memory
protections.

This RFC seeks your feedback regarding introducing an interface that
allows dynamic configuration of memory protection settings.

I would like to propose two options:
1. Describing the memory protection setting configuration in a HOB that
is produced by the platform.
2. Introducing a library class (e.g. MemoryProtectionLib) that allows
abstraction of the memory protection setting configuration data source.

In addition, I would like to know if the memory protection FixedAtBuild
PCDs currently in MdeModulePkg can be removed so we can move the
configuration interface entirely to an option above.

In any case, I would like the settings to be visible to environments
such as Standalone MM where dynamic PCDs are not accessible.

I am seeking your feedback on this proposal in preparation for sending
an edk2 patch series.

--
Taylor Beebe
Software Engineer @ Microsoft
--
Taylor Beebe
Software Engineer @ Microsoft
--
Taylor Beebe
Software Engineer @ Microsoft


--
Taylor Beebe
Software Engineer @ Microsoft


Re: edk2 memory map on QEMU

Stuart Yoder
 

On 7/30/21 3:36 PM, Andrew Fish wrote:

On Jul 30, 2021, at 12:32 PM, Stuart Yoder <stuart.yoder@arm.com <mailto:stuart.yoder@arm.com>> wrote:

I am playing around with EDK2 on QEMU with a UEFI shell application and in the app I allocate some memory using gBS->AllocatePool(EfiBootServicesData, ...)

Programmatically accessing the pointer returned works fine, but when I print it, it does not seem to be what I would expect is a valid address.

I've allocated 4GB to the QEMU machine, which I believe starts at 0x40000000.
You can run the `memmap` command at the EFI Shell to see the layout.

But, when I print the address returned by AllocatePool the value is "0x39177018".
Print != printf on some of the format string so be careful about that….
I figured it out. It was a dumb Print() formatting error that resulted in
the address getting truncated to 8 bytes.

Thanks,
Stuart


Re: EmulatorPkg and the state of DlLoadImage()

Andrew Fish
 



On Jul 30, 2021, at 1:34 PM, Marvin Häuser <mhaeuser@...> wrote:

30.07.2021 17:58:05 Andrew Fish via groups.io <afish@...>:



On Jul 30, 2021, at 3:37 AM, Marvin Häuser <mhaeuser@...> wrote:

Good day everyone,

I'm currently refining the port of EmulatorPkg to my new PE/COFF loader library instance.
In the process, I found the function DlOpenImage() [1], which loads UEFI Images via the OS loader to utilise its symbol loading capability. Theoretically, this should e.g. allow arbitrary debuggers using the OS APIs to symbolise the backtrace.

macOS: The function seems to be unused entirely. [2]

Linux: On my system running Fedora 34, the function neither works out-of-the-box, nor after significant time of trying to fix it. The first issue is that it only proceeds if the Image has a PDB path with ".pdb" extension [3], while the GCC5 toolchain generates Images with ".dll" files for PDB paths (see errors below). Once this is resolved, there is an error message indicating insufficient Image section alignment:

[...]/Build/EmulatorX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/EbcDxe/EbcDxe/DEBUG/EbcDxe.dll: ELF load command alignment not page-aligned


The requiring *.pdb seems like something that rotted out and could be fixed. 

OK, yes.

Resolving this yields an error that executable files cannot be loaded dynamically:

[...]/Build/EmulatorX64/DEBUG_GCC5/X64/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.dll: cannot dynamically load executable

With my very limited knowledge about Linux and ELF I tried the naive approach of building the Images as shared (hoping it would be similar to DLLs, which are built on Windows), but this just silently crashes.


This code is very very old. Notice the comment about gdb predates gdb Python support [1].

Right, that is why I wondered, whether it is still needed.
What happens if you comment out the DlLoadImage path? There seems to be some gdb scripts?

It already is pretty much no-op on my system. On break, the debug code writes the symbol file paths into a file (Host.gdb) and a gdb script loads them. With gdb, this code seems to not be needed at all.

The macOS path sets breakpoints on SecGdbScriptBreak() in an lldb script and loads symbols via that path. That his probably the best path forward for gdb too? 
It looks like if you `build.sh run` you should launch the emulator under gdb and source the symbol loading file.
EmulatorPkg/build.sh:221:  /usr/bin/gdb $BUILD_ROOT_ARCH/Host -q -cd=$BUILD_ROOT_ARCH -x $WORKSPACE/EmulatorPkg/Unix/*GdbRun.sh*

If you comment out the dlopen() path does it start working? Looks like breaking in with gdb should get symbols loaded? 

Yes, it works, even without uncommenting. I'm not experienced with gdb enough to know how it should work, but I could help with a better solution if such is needed later. For now, I'd like to figure out whether dlopen() is legacy code nobody uses or needs first.


Marvin,

Sorry I started looking at the code and forgot to give the history. The dlopen() was a trick copied from the Windows port. Basically an EFI image is loaded into EFI memory and it is also loaded into the application memory outside of EFI vis the dlopen() and the entry point is returned to be the dlopen() one. This fakes EFI out enough to be happy, and since the EFI code was loaded via dlopen() symbols get loaded automatically. So the only purpose of the dlopen is automatic source level debugging without requiring any debugger scripting. 

The gdb/lldb scripts have magic to tell the debugger the location things EFI got loaded, so you don’t need the dlopen() to load symbols. Basically not using the dlopen is a more realistic simulation of EFI. 


So my questions are:
1) Does this code currently work for anyone?
2) Does anyone use a debugging setup that is incompatible with Images loaded by EDK II rather than the OS?

Not a 100% sure what you are asking?

Sorry if it was unclear, "loaded by EDK II" refers to Images loaded without dlopen() and "loaded by the OS" to Images loaded with dlopen().


Hopefully the above history clears that up.

In a lot of cases you are debugging what is compatible with the OS? For example on macOS we build a mach-O and convert that to PE/COFF. We point the PDB entry at the mach-O file and that is what the debugger sees. As long as the PE/COFF lines up with the mach-O it does not really matter, as at the end of the day the debugger is just processing the dwarf debug info associated with addresses in system memory. 
3) Are the issues above known and planned to be fixed?


Not likely please file a BZ. 

OK, will do soon.


Note I’m working on getting a generic gdb debugging script into the edk2 [2] and that should also work with the Emulator. I think you could replace the ` -x $WORKSPACE/EmulatorPkg/Unix/GdbRun.sh` with `-ex efi_gdb.py’. There is not a break hook in those scripts so you would have to run the `efi` command the 1st time you attach to load symbols. The efi_gdb.py script works on stock EFI so it does not depend on any of the hooks in the EmulatorPkg to work. 

I'm not good with Python, so will have to check. :)

I was forced to learn Python to figure about debugging scripts :)

Thanks,

Andrew Fish


Best regards,
Marvin


Thank you for your time!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/be282b14938846960cce30825a9fe762e14ca8c9/EmulatorPkg/Unix/Host/Host.c#L1065-L1113

[2]
https://github.com/tianocore/edk2/blob/be282b14938846960cce30825a9fe762e14ca8c9/EmulatorPkg/Unix/Host/Host.c#L1071-L1073

[3]
https://github.com/tianocore/edk2/blob/be282b14938846960cce30825a9fe762e14ca8c9/EmulatorPkg/Unix/Host/Host.c#L1084-L1086
https://github.com/tianocore/edk2/blob/be282b14938846960cce30825a9fe762e14ca8c9/EmulatorPkg/Unix/Host/Host.c#L1003-L1026


[1] https://github.com/tianocore/edk2/blob/be282b14938846960cce30825a9fe762e14ca8c9/EmulatorPkg/Unix/Host/Host.c#L1179
[2] https://github.com/ajfish/edk2/blob/BZ3500-gdb/efi_gdb.py

Thanks,

Andrew Fish




6121 - 6140 of 84522