1. devel@edk2.groups.io
  2. Messages
   Date   
[PATCH v1 0/3] Fix possible uninitialized uses

Sergei Dmitrouk <sergei@...>
 

v1:

Compiling for IA32 target with gcc-5.5.0 emits "maybe-uninitialized" warnings.
Compilation command: build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t GCC49

Unlike other cases mentioned in
https://bugzilla.tianocore.org/show_bug.cgi?id=3228
these seem to be actual issues in the code. Read patches for specifics.

v2:

Second patch was simplified.

Sergei Dmitrouk (3):
ShellPkg/HttpDynamicCommand: Fix possible uninitialized use
MdeModulePkg/PciBusDxe: Fix possible uninitialized use
CryptoPkg/BaseCryptLib: Fix possible uninitialized use

CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c | 1 +
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 5 ++---
ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c | 1 +
4 files changed, 5 insertions(+), 3 deletions(-)

--
2.17.6
Re: 回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

Sergei Dmitrouk <sergei@...>
 

Yes, adding `-ffat-lto-objects` makes the warning appear even with GCC 5.5.0.

Regards,
Sergei

On Tue, May 18, 2021 at 08:59:05AM +0800, gaoliming wrote:
Sergei:
Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool chain.
They both work on the different GCC version, such as gcc5, gcc6..

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
-ffat-lto-objects option that can trig the warning with LTO option. Do you
try it?

If this option works, we can update GCC5 tool chain definition in
tools_def.txt, then this issue can be detected in CI GCC5 build.

Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
Dmitrouk
发送时间: 2021年5月15日 21:01
收件人: devel@edk2.groups.io; jiewen.yao@...
抄送: Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX
<xiaoyux.lu@...>; Jiang, Guomin <guomin.jiang@...>
主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
uninitialized use

Hello Jiewen,

I get the error only for GCC49 and not for GCC5 toolchain. CI uses GCC5.

So I compared build commands and this seems to depend on LTO. Adding
`-flto`
impedes compiler's ability to detect such simple issues.

I've found relevant bug report, there is even fix suggestion from last
month:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844

Regards,
Sergei

On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
Hi Sergei
Thank you very much for the fix.
Reviewed-by: Jiewen Yao <Jiewen.yao@...>

I am a little surprised why it is not caught before. It is an obvious
logic issue.

Do you think we can do anything on CI, to catch it during pre-check-in
in the
future?
I just feel it is burden to make it post-check-in fix.


Thank you
Yao Jiewen

-----Original Message-----
From: Sergei Dmitrouk <sergei@...>
Sent: Friday, May 14, 2021 8:17 PM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>;
Lu, XiaoyuX <xiaoyux.lu@...>; Jiang, Guomin
<guomin.jiang@...>
Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
uninitialized
use

`Result` can be used uninitialized in both functions after following
either first or second `goto` statement.

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Xiaoyu Lu <xiaoyux.lu@...>
Cc: Guomin Jiang <guomin.jiang@...>
Signed-off-by: Sergei Dmitrouk <sergei@...>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c | 1 +
CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
index 4009d37d5f91..0b2960f06c4c 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
@@ -82,6 +82,7 @@ RsaPssVerify (
EVP_PKEY_CTX *KeyCtx;
CONST EVP_MD *HashAlg;

+ Result = FALSE;
EvpRsaKey = NULL;
EvpVerifyCtx = NULL;
KeyCtx = NULL;
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
index b66b6f7296ad..ece765f9ae0a 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
@@ -97,6 +97,7 @@ RsaPssSign (
EVP_PKEY_CTX *KeyCtx;
CONST EVP_MD *HashAlg;

+ Result = FALSE;
EvpRsaKey = NULL;
EvpVerifyCtx = NULL;
KeyCtx = NULL;
--
2.17.6
Re: [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure

Laszlo Ersek
 

On 05/17/21 20:25, Erdem Aktas wrote:
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00

Just one question: is there any reason why GHCB_* defines are decimal
while the SVM_EXIT_* are all in hexadecimal? Does EDK2 have any
preference?
(I'm unaware of any preference in edk2 -- it's probably best to stick
with the base that the spec itself uses, but even using a different base
is not a huge deal, if the numbers in question are not large.)

Thanks!
Laszlo


Reviewed-by: Erdem Aktas <erdemaktas@...>

-Erdem

On Wed, May 12, 2021 at 4:46 PM Brijesh Singh <brijesh.singh@...> wrote:

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

An SEV-SNP guest is required to perform the GHCB GPA registration. See
the GHCB specification for further details.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Reviewed-by: Laszlo Ersek <lersek@...>
Reviewed-by: Liming Gao <gaoliming@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index cdb8f588ccf8..542e4cdf4782 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -53,6 +53,11 @@ typedef union {
UINT64 Features:52;
} GhcbHypervisorFeatures;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:52;
+ } GhcbGpaRegister;
+
VOID *Ghcb;

UINT64 GhcbPhysicalAddress;
@@ -62,6 +67,8 @@ typedef union {
#define GHCB_INFO_SEV_INFO_GET 2
#define GHCB_INFO_CPUID_REQUEST 4
#define GHCB_INFO_CPUID_RESPONSE 5
+#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
+#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
--
2.17.1
Re: [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation

Laszlo Ersek
 

On 05/17/21 16:17, Tom Lendacky wrote:
On 5/16/21 10:08 PM, Laszlo Ersek wrote:
Patches v2 01-05 look good to me, thanks for the updates. Now on to this
one:

On 05/13/21 01:46, Brijesh Singh wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3D3275&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C55d74e19a5554988e0b608d918e1215c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568177488739589%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ZIy6xEXWmxgVxZm6mrdlroM7OPQajEjUSD8W5z2ohu0%3D&amp;reserved=0
(1) The "3D" seems like a typo in the bug ticket URL. (This crept into
v2 somehow; v1 didn't have it.)


Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
in the guest VM. See the GHCB specification, Table 5 "List of Supported
Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.

While at it, define the VMSA state save area that is required for creating
the AP. The save area format is defined in AMD APM volume 2, Table B-4
(there is a mistake in the table that defines the size of the reserved
area at offset 0xc8 as a dword, when it is actually a word). The format of
the save area segment registers is further defined in AMD APM volume 2,
sections 10 and 15.5.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Reviewed-by: Liming Gao <gaoliming@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
MdePkg/Include/Register/Amd/Ghcb.h | 76 ++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 029904b1c63a..4d1ee29e0a5e 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -55,6 +55,7 @@
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
+#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL

@@ -83,6 +84,12 @@
#define IOIO_SEG_ES 0
#define IOIO_SEG_DS (BIT11 | BIT10)

+//
+// AP Creation Information
+//
+#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
+#define SVM_VMGEXIT_SNP_AP_CREATE 1
+#define SVM_VMGEXIT_SNP_AP_DESTROY 2

typedef PACKED struct {
UINT8 Reserved1[203];
@@ -195,4 +202,73 @@ typedef struct {
SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
} SNP_PAGE_STATE_CHANGE_INFO;

+//
+// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
+// Only the fields required to be set to a non-zero value are defined.
+//
+#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
+#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
+
+#define SEV_ES_RESET_LDT_TYPE 0x2
+#define SEV_ES_RESET_TSS_TYPE 0x3
+
+#pragma pack (1)
+typedef union {
+ struct {
+ UINT16 Type:4;
+ UINT16 Sbit:1;
+ UINT16 Dpl:2;
+ UINT16 Present:1;
+ UINT16 Avl:1;
+ UINT16 Reserved1:1;
+ UINT16 Db:1;
+ UINT16 Granularity:1;
+ } Bits;
+ UINT16 Uint16;
+} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
+
+typedef struct {
+ UINT16 Selector;
+ SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
+ UINT32 Limit;
+ UINT64 Base;
+} SEV_ES_SEGMENT_REGISTER;
+
I'm not saying anything is incorrect about this, but I *am* going to
rant about the APM.
Yes, the APM is definitely lacking in this area.


It's simply impenetrable. I've been staring at it for ~50 minutes now,
and I still cannot fully connect it to your code.

[1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
virtualized, not SMM) segment descriptors. Why *these* are relevant
*here* is nothing short of mind-boggling, but please bear with me.

[2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
Architecture SMM State-Save Area", the reader is introduced to the
2+2+4+8 segment register representation. The table only lists "Selector,
Attributes, Limit, Base" as fields, and nothing about the actual
contents. Way too little information. I guess this is covered by the
commit message reference "section 10".

[3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
paragraph "Segment State in the VMCB", we're given a long-winded,
*textual* only -- no diagram! -- and *differential* (relative)
explanation, on top of the two, above-quoted, locations of the spec. I'm
sorry but this is just impossible to follow. Would it have been a
unaffordable to insert a self-contained diagram here, with
self-contained, absolute explanation?

So let me quote:

The segment registers are stored in the VMCB in a format similar to
that for SMM: both base and limit are fully expanded; segment
attributes are stored as 12-bit values formed by the concatenation
of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
descriptors; the descriptor “P” bit is used to signal NULL segments
(P=0) where permissible and/or relevant.

So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
fields of the SMM and VMCB segment register representation are
explained. Further, we get the following for the Attributes field, by
manual reconstruction:

55 54 53 52 47 46 45 44 43 42 41 40

G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
* * * * * * ignored bits in 64-bit mode

G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
* * * * * * * * * * ignored bits in 64-bit mode

In other words, in the code segment descriptor, D, L, P, DPL, and C
matter. In the data segment descriptor, only P matters.
The APs won't be in 64-bit mode when being started. In reset, they will be
in real mode, so the attributes will be set up for that. Please see Table
4-3 and Table 4-4 for how these will matter.


In particular, what [3] says, quoted above, about the "P" bit, seems
sensible -- "P" is indeed *not* ignored for either segment descriptor
type (code or data).

But then we continure reading [3], and we find (quote again):

The loading of segment attributes from the VMCB (which may have been
overwritten by software) may result in attribute bit values that are
otherwise not allowed. However, only some of the attribute bits are
actually observed by hardware, depending on the segment register in
question:
* CS—D, L, P, and R.
* SS—B, P, E, W, and Code/Data
* DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
* LDTR—P, S, and Type (LDT)
* TR—P, S, and Type (32- or 16-bit TSS)

I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.

For CS, the spec says, "D, L, P, and R" are observed by the hardware.
But we've just shown that R is ignored *in general* for code segments in
64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
P, DPL, C }.

For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
observed. I'm going to give "Code/Data" a pass (bit 43 in the original
representation), but the rest is {D, P, DPL, E, W}, which is *not a
subset* of { P }.

Again, from [1], section 4.8.2 specifically, E (expand-down) and W
(writeable) are ignored, DPL is ignored, and D isn't even called D but
"D/B", and it is ignored. So how can the spec say in [3] that the
hardware observes { D, DPL, E, W } when all those are ignored per prior
definition [1]?

- And then I see no reference that SEV-ES uses the same
(circumstantially-defined) segment register format.


So anyway, I'll just accept that I don't understand the spec -- I think
it has inconsistencies. Let's look at the code:

- Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
for the code segment descriptor, and [0,E,W,A] for data segment descriptors

SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which maps
to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
have no clue why we set it.
For reset, when we are in real mode, that would correspond to executable /
readable type.


(2) Can you please explain that? Just in this discussion thread, no need
ot change the code or the commit message.
The idea is that we need to support real mode for the AP creation support,
but actually AP creation isn't limited to that. I didn't want to
overly-define the segment register for all the different modes, since it
would only be real mode that would be used by OVMF, but maybe that would
make it eaiser / clearer to understand...


The C ("conforming") bit actually matters. It is documented in section
"4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
It seems like "non-conforming" is not a problem here, as long as we
don't use multiple privilege levels, which I think we don't, in
firmware-land. OK.

Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.

(3) Why is W (writeable) set to 1, when, according to [1], it is ignored
in 64-bit mode?
Same as previous comment, the AP will be starting in real mode.



- "Sbit" seems to stand for bit 44 from the original representation --
constant 1. OK I think.

- "Dpl", "Present" and "Avl" look OK to me.

- Should "Reserved1" really be called that? It seems to map to bit 53 in
the original representation, and the L (long mode) bit actually matters
for the code segment. (It indeed appears undefined / reserved for data
segments.)

Am I *totally* mistaken here and we're not even talking long mode?
:)


- "Db" and "Granularity" look OK.

- SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.

- SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
Descriptor", "The TSS descriptor is a system-segment descriptor", which
makes me think that I'm looking at value 3 in the proper table (Table 4-6).
I'll add a reference to table 14-2 under the "Processor Initialization"
section.



(4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?
For processor reset, type 3 represents a busy 16-bit TSS.

So I can work on clarifying the comments around all of the definitions and
indicate that values are defined for processor reset support and that more
definitions would be needed if the segment registers want to be examined /
set in different modes. Thoughts?
My bad -- I feel really dumb now.

The four important macros which I got hung upon are all named
SEV_ES_RESET_*. Keyword being "reset". :/

With the typo (1) fixed:

Reviewed-by: Laszlo Ersek <lersek@...>

Thanks & sorry!
Laszlo



Thanks,
Tom


(Please note that this is only superficial pattern matching from my side
("TSS"); I don't know the first thing about "hardware task management".)


+typedef struct {
+ SEV_ES_SEGMENT_REGISTER Es;
+ SEV_ES_SEGMENT_REGISTER Cs;
+ SEV_ES_SEGMENT_REGISTER Ss;
+ SEV_ES_SEGMENT_REGISTER Ds;
+ SEV_ES_SEGMENT_REGISTER Fs;
+ SEV_ES_SEGMENT_REGISTER Gs;
+ SEV_ES_SEGMENT_REGISTER Gdtr;
+ SEV_ES_SEGMENT_REGISTER Ldtr;
+ SEV_ES_SEGMENT_REGISTER Idtr;
+ SEV_ES_SEGMENT_REGISTER Tr;
+ UINT8 Reserved1[42];
+ UINT8 Vmpl;
+ UINT8 Reserved2[5];
+ UINT64 Efer;
+ UINT8 Reserved3[112];
+ UINT64 Cr4;
+ UINT8 Reserved4[8];
+ UINT64 Cr0;
+ UINT64 Dr7;
+ UINT64 Dr6;
+ UINT64 Rflags;
+ UINT64 Rip;
+ UINT8 Reserved5[232];
+ UINT64 GPat;
+ UINT8 Reserved6[320];
+ UINT64 SevFeatures;
+ UINT8 Reserved7[48];
+ UINT64 XCr0;
+ UINT8 Reserved8[24];
+ UINT32 Mxcsr;
+ UINT16 X87Ftw;
+ UINT8 Reserved9[2];
+ UINT16 X87Fcw;
+} SEV_ES_SAVE_AREA;
+#pragma pack ()
+
#endif
This part looks good to me.

(I spent almost two hours reviewing this patch.)

Thanks!
Laszlo
Re: [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area

Laszlo Ersek
 

On 05/17/21 00:15, Marvin Häuser wrote:
Am 16.05.2021 um 03:17 schrieb Laszlo Ersek:
On 05/14/21 17:44, Marvin Häuser wrote:
On 14.05.21 17:23, Lendacky, Thomas wrote:
On 5/14/21 10:04 AM, Marvin Häuser wrote:
+      // Check to be sure that the "allocate below" behavior hasn't
changed.
+      // This will also catch a failed allocation, as "-1" is
returned on
+      // failure.
+      //
+      if (CpuMpData->SevEsAPResetStackStart >=
CpuMpData->WakeupBuffer) {
+        DEBUG ((DEBUG_ERROR,
+          "SEV-ES AP reset stack is not below wakeup buffer\n"));
+
+        ASSERT (FALSE);
Should the ASSERT not only catch the broken "allocate below"
behaviour,
i.e. not trigger on failed allocation?
I think it's best to trigger on a failed allocation as well rather than
continuing and allowing a page fault or some other problem to occur.
Well, it should handle the error in a safe way, i.e. the deadloop below.
To not ASSERT on plausible conditions is a common design guideline in
most low-level projects including Linux kernel.

Best regards,
Marvin

Thanks,
Tom

+        CpuDeadLoop ();
"DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.

In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
-- don't continue execution if the condition controlling the whole block
fired.

In DEBUG and NOOPT builds, the pattern will lead to a debug message
(usually at the "error" level), followed by an assertion failure. The
error message of the assertion failure is irrelevant ("FALSE"). The
point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
hangs execution is customizable, via "PcdDebugPropertyMask", unlike
CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
effect is the same -- the explicit CpuDeadLoop is not reached. In other
configs, ASSERT() can raise a debug exception (CpuBreakpoint()).
I absolutely do not *expect* Tom to change this, it was just a slight
remark (as many places have this anyway). I'll still try to explain why
I made that remark, but for whom it is of no interest, I do not expect
it to be read. I'm fine with the patch as-is myself. Thank you a lot, Tom!



I know it, unfortunately, is a pattern in EDK II - taking this pattern
too far is what caused the 8-revision patch regarding untrusted inputs
we submitted previously. :)

There are many concerns about unconventional ASSERTs, though I must
admit none but one (and that one barely) really apply here, which is why
I have trouble explaining why I believe it should be changed. Here are
some reasons outside the context of this patch:

1) Consistency between DEBUG and RELEASE builds: I think one can justify
to have a breakpoint on a condition that may realistically occur. But a
deadloop can give a wrong impression about how production code works.
E.g. it also is a common pattern in EDK II to ASSERT on memory
allocation failure but *not* have a proper check after, so DEBUG builds
will nicely error or deadloop, while RELEASE goes ahead and causes a CPU
exception or memory corruption depending on the context. Thus,
real-world error handling cannot really be tested. This does not apply
because there *is* a RELEASE deadloop.

2) Static analysis: Some static analysers use ASSERT information for
their own analysis, and try to give hints about unsafe or unreachable
code based on own annotations. This kind of applies, but only when
substituting EDK II ASSERT with properly recognisable ASSERTs (e.g.
__builtin_unreachable).

2) Dynamic analysis: ASSERTs can be useful when fuzzing for example.
Enabled Sanitizers will only catch unsafe behaviour, but maybe you have
some extra code in place to sanity-check the results further. An ASSERT
yields an error dump (usually followed by the worker dying). However, as
allocation failures are perfectly expected, this can cause a dramatic
about of False Positives and testing interruption. This does not apply
because deadloop'd code cannot really be fuzz-tested anyway.

ASSERTs really are designed as unbreakable conditions, i.e. 1)
preconditions 2) invariants 3) postconditions. No allocator in early
kernel-space or lower can really guarantee allocation success, thus it
cannot be a postcondition of any such function. And while it might make
debugging look a bit easier (but you will see from the backtrace anyway
where you halted), it messes with all tools that assume proper usage.

Also, I just realised, you can of course see it from the address value
when debugging, but you cannot see it from the ASSERT or DEBUG message
*which* of the two logical error conditions failed (i.e. broken
allocator or OOM). Changing the ASSERT would fix that. :)
I'm OK if the ASSERT() is dropped; from my perspective it's really just
a small convenience / developer/debugging aid. We still have the debug
message and the explicit deadloop.

Thanks
Laszlo
Re: [edk2-platforms][PATCH v2 6/6] Platform/StandaloneMm: build StandaloneMmRpmb for 32bit architectures

Sami Mujawar
 

Hi Etienn,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 06:50 AM, Etienne Carriere wrote:
Build PlatformStandaloneMmRpmb for ARM architecture (32bit arm machine).
The generated image targets an execution environment similar to AArch64
StMM secure partition in OP-TEE but in 32bit mode.

GCC flag -fno-stack-protector
added. The stack protection code bring
GOT dependencies we prefer avoid when StMM runs in OP-TEE.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- Remove useless duplication of ArmSvcLib loading.
- Move BaseStackCheckLib to generic library classes instead of ARM only.
- include MdePkg/MdeLibs.dsc.inc
instead of loading
RegisterFilterLibNull.inf for ARM architecture.
---
Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc b/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
index cb3f1ddf52..33364deb1e 100644
--- a/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
+++ b/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
@@ -16,12 +16,14 @@
PLATFORM_VERSION = 1.0
DSC_SPECIFICATION = 0x0001001C
OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME)
- SUPPORTED_ARCHITECTURES = AARCH64
+ SUPPORTED_ARCHITECTURES = ARM|AARCH64
BUILD_TARGETS = DEBUG|RELEASE|NOOPT
SKUID_IDENTIFIER = DEFAULT
FLASH_DEFINITION = Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.fdf
DEFINE DEBUG_MESSAGE = TRUE
+!include MdePkg/MdeLibs.dsc.inc
+
################################################################################
#
# Library Class section - list of all Library Classes needed by this Platform.
@@ -39,6 +41,7 @@
FvLib|StandaloneMmPkg/Library/FvLib/FvLib.inf
HobLib|StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+ NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
@@ -68,6 +71,9 @@
#
NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
+[LibraryClasses.ARM]
+ ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
+
[LibraryClasses.common.MM_STANDALONE]
HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
@@ -160,3 +166,7 @@
[BuildOptions.AARCH64]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv8-a+nofp
GCC:*_*_*_CC_FLAGS = -mstrict-align
+
+[BuildOptions.ARM]
+GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv7-a
+GCC:*_*_*_CC_FLAGS = -fno-stack-protector
Re: [edk2-platforms][PATCH v2 5/6] Drivers/OpTee: address cast build warning issue in 32b mode

Sami Mujawar
 

Hi Etienn,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 06:50 AM, Etienne Carriere wrote:
Use (UINTN) cast to cast physical or virtual address values to the
pointer size before casting from/to a pointer value.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v1
---
Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c | 21 +++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c b/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c
index 6eb19bed0e..83c2750368 100644
--- a/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c
+++ b/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c
@@ -305,7 +305,8 @@ OpTeeRpmbFvbRead (
}
}
- Base = (VOID *)Instance->MemBaseAddress + (Lba * Instance->BlockSize) + Offset;
+ Base = (VOID *)(UINTN)Instance->MemBaseAddress + (Lba * Instance->BlockSize) +
+ Offset;
// We could read the data from the RPMB instead of memory
// The 2 copies should already be identical
// Copy from memory image
@@ -387,7 +388,8 @@ OpTeeRpmbFvbWrite (
return Status;
}
}
- Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset;
+ Base = (VOID *)(UINTN)Instance->MemBaseAddress + (Lba * Instance->BlockSize) +
+ Offset;
Status = ReadWriteRpmb (
SP_SVC_RPMB_WRITE,
(UINTN)Buffer,
@@ -477,7 +479,8 @@ OpTeeRpmbFvbErase (
return EFI_INVALID_PARAMETER;
}
NumBytes = NumLba * Instance->BlockSize;
- Base = (VOID *)Instance->MemBaseAddress + Start * Instance->BlockSize;
+ Base = (VOID *)(UINTN)Instance->MemBaseAddress +
+ (Start * Instance->BlockSize);
Buf = AllocatePool (NumLba * Instance->BlockSize);
if (Buf == NULL) {
return EFI_DEVICE_ERROR;
@@ -689,7 +692,7 @@ InitializeFvAndVariableStoreHeaders (
goto Exit;
}
// Install the combined header in memory
- CopyMem ((VOID*)Instance->MemBaseAddress, Headers, HeadersLength);
+ CopyMem ((VOID*)(UINTN)Instance->MemBaseAddress, Headers, HeadersLength);
Exit:
FreePool (Headers);
@@ -747,14 +750,18 @@ FvbInitialize (
// Read the file from disk and copy it to memory
ReadEntireFlash (Instance);
- FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->MemBaseAddress;
+ FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)Instance->MemBaseAddress;
Status = ValidateFvHeader (FwVolHeader);
if (EFI_ERROR (Status)) {
// There is no valid header, so time to install one.
DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__));
// Reset memory
- SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL);
+ SetMem64 (
+ (VOID *)(UINTN)Instance->MemBaseAddress,
+ Instance->NBlocks * Instance->BlockSize,
+ ~0UL
+ );
DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__));
Status = ReadWriteRpmb (
SP_SVC_RPMB_WRITE,
@@ -827,7 +834,7 @@ OpTeeRpmbFvbInit (
mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite;
mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead;
- mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS)Addr;
+ mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Addr;
mInstance.Signature = FLASH_SIGNATURE;
mInstance.Initialize = FvbInitialize;
mInstance.BlockSize = EFI_PAGE_SIZE;
Re: [edk2-platforms][PATCH v2 4/6] Drivers/OpTee: Add Aarch32 SVC IDs for 32bit Arm targets

Sami Mujawar
 

Hi Etienn,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 06:50 AM, Etienne Carriere wrote:
Add SMCCC function IDs for RPMB read/write service on 32bit architectures.
Define generic SP_SVC_RPMB_READ/SP_SVC_RPMB_WRITE IDs for native target
architecture (32b or 64b).

Changes OpTeeRpmbFvb.c to use architecture agnostic macro
ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ for 32b and 64b support.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- Use _AARCH64 (resp. _AARCH32) suffix instead of _64 (resp. _32) in
the added macros.
---
Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c | 2 +-
Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.h | 16 ++++++++++++++--
2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c b/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c
index 5197c95abd..6eb19bed0e 100644
--- a/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c
+++ b/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c
@@ -68,7 +68,7 @@ ReadWriteRpmb (
ZeroMem (&SvcArgs, sizeof (SvcArgs));
- SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+ SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
SvcArgs.Arg1 = mStorageId;
SvcArgs.Arg2 = 0;
SvcArgs.Arg3 = SvcAct;
diff --git a/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.h b/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.h
index c17fc287ef..9c2a4ea6a5 100644
--- a/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.h
+++ b/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.h
@@ -13,8 +13,20 @@
contract between OP-TEE and EDK2.
For more details check core/arch/arm/include/kernel/stmm_sp.h in OP-TEE
**/
-#define SP_SVC_RPMB_READ 0xC4000066
-#define SP_SVC_RPMB_WRITE 0xC4000067
+#define SP_SVC_RPMB_READ_AARCH64 0xC4000066
+#define SP_SVC_RPMB_WRITE_AARCH64 0xC4000067
+
+#define SP_SVC_RPMB_READ_AARCH32 0x84000066
+#define SP_SVC_RPMB_WRITE_AARCH32 0x84000067
+
+#ifdef MDE_CPU_AARCH64
+#define SP_SVC_RPMB_READ SP_SVC_RPMB_READ_AARCH64
+#define SP_SVC_RPMB_WRITE SP_SVC_RPMB_WRITE_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define SP_SVC_RPMB_READ SP_SVC_RPMB_READ_AARCH32
+#define SP_SVC_RPMB_WRITE SP_SVC_RPMB_WRITE_AARCH32
+#endif
#define FLASH_SIGNATURE SIGNATURE_32 ('r', 'p', 'm', 'b')
#define INSTANCE_FROM_FVB_THIS(a) CR (a, MEM_INSTANCE, FvbProtocol, \
Re: [edk2-platforms][PATCH v2 3/6] Platform/StandaloneMm: sync with edk2 StandaloneMmCpu path change

Sami Mujawar
 

Hi Etienn,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 06:50 AM, Etienne Carriere wrote:
Synchronize with edk2 package where StandaloneMmCpu component has moved
from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Cc: Thomas Abraham <thomas.abraham@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- split change in 3: this change relates to StandaloneMm package only.
---
Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc | 2 +-
Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.fdf | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc b/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
index f99a47ebf6..cb3f1ddf52 100644
--- a/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
+++ b/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
@@ -133,7 +133,7 @@
#
Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFv.inf
StandaloneMmPkg/Core/StandaloneMmCore.inf
- StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf {
<LibraryClasses>
NULL|Drivers/OpTee/OpteeRpmbPkg/FixupPcd.inf
diff --git a/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.fdf b/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.fdf
index e175dc7b2d..c4295a3e63 100644
--- a/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.fdf
+++ b/Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.fdf
@@ -68,7 +68,8 @@ READ_LOCK_STATUS = TRUE
INF Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFv.inf
INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
- INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ INF StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
+
################################################################################
#
# Rules are use with the [FV] section's module INF type to define
Re: [edk2-platforms][PATCH v2 2/6] Platform/Socionext/DeveloperBox: sync with edk2 StandaloneMmCpu path change

Sami Mujawar
 

Hi Etienn,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 06:50 AM, Etienne Carriere wrote:
Synchronize with edk2 package where StandaloneMmCpu component has moved
from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Cc: Thomas Abraham <thomas.abraham@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- split change in 3: this change relates to DeveloperBox only.
---
Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc | 2 +-
Platform/Socionext/DeveloperBox/DeveloperBoxMm.fdf | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc b/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc
index e078de4bbb..b5524f87a6 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBoxMm.dsc
@@ -80,7 +80,7 @@
gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x2
}
- StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/Fip006StandaloneMm.inf
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf {
diff --git a/Platform/Socionext/DeveloperBox/DeveloperBoxMm.fdf b/Platform/Socionext/DeveloperBox/DeveloperBoxMm.fdf
index 33de03c8e7..89453477c9 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBoxMm.fdf
+++ b/Platform/Socionext/DeveloperBox/DeveloperBoxMm.fdf
@@ -111,7 +111,7 @@ READ_LOCK_STATUS = TRUE
INF Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/Fip006StandaloneMm.inf
INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
- INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ INF StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
################################################################################
#
Re: [edk2-platforms][PATCH v2 1/6] Platform/ARM/SgiPkg: sync with edk2 StandaloneMmCpu path change

Sami Mujawar
 

Hi Etienn,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 06:50 AM, Etienne Carriere wrote:
Synchronize with edk2 package where StandaloneMmCpu component has moved
from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Ilias Apalodimas <ilias.apalodimas@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Cc: Thomas Abraham <thomas.abraham@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v1:
- split change in 3: this change relates to Platform/ARM/SgiPkg only.
---
Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc | 2 +-
Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc b/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
index e281d54909..1e0af23711 100644
--- a/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
+++ b/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
@@ -122,7 +122,7 @@
StandaloneMmPkg/Core/StandaloneMmCore.inf
[Components.AARCH64]
- StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
###################################################################################################
#
diff --git a/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf b/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
index 5a0772cd85..96b4272dd6 100644
--- a/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
+++ b/Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
@@ -49,7 +49,7 @@ READ_LOCK_CAP = TRUE
READ_LOCK_STATUS = TRUE
INF StandaloneMmPkg/Core/StandaloneMmCore.inf
- INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+ INF StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
################################################################################
#
Re: GSoC 2021 Qemu OpenBoardPkg Project

KAAIRA GUPTA
 

On Tue, May 18, 2021 at 08:01:57PM +0530, Kaaira Gupta wrote:
Hey everyone,

I have been selected as a student developer for the project MinPlatform
Qemu OpenBoardPkg under the mentors Ray Ni and Michael Kubacki. Thankyou
for this opportunity. I have been over the major chapters of Beyond BIOS
as recommended by Nate DeSimone. I want to get familiar with the code
now to help me undersatnd the community practices and get my hands
dirty. Where should I start? What development environment do I need?
How can I use this community bonding period to give me a better start
for the coding phase?

How do the mentors want me to connect with them? Can we have a meet to
discuss this project's plan to add more details? This would be very
helpful for me considering I don't have prior experience with EDK2.
I noticed that the mail-id that I have used of Michael Kubacki doesn't
exist anymore. Please let me know how I can contact him.


Thank you,
Kaaira
Thanks,
Kaaira
GSoC 2021 Qemu OpenBoardPkg Project

KAAIRA GUPTA
 

Hey everyone,

I have been selected as a student developer for the project MinPlatform
Qemu OpenBoardPkg under the mentors Ray Ni and Michael Kubacki. Thankyou
for this opportunity. I have been over the major chapters of Beyond BIOS
as recommended by Nate DeSimone. I want to get familiar with the code
now to help me undersatnd the community practices and get my hands
dirty. Where should I start? What development environment do I need?
How can I use this community bonding period to give me a better start
for the coding phase?

How do the mentors want me to connect with them? Can we have a meet to
discuss this project's plan to add more details? This would be very
helpful for me considering I don't have prior experience with EDK2.

Thank you,
Kaaira
Re: [PATCH v3 5/5] StandaloneMmPkg: build for 32bit arm machines

Sami Mujawar
 

Hi Etienne,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 17/05/2021 08:40 AM, Etienne Carriere wrote:
This change allows to build StandaloneMmPkg components for 32bit Arm
StandaloneMm firmware.

This change mainly moves AArch64/ source files to Arm/ side directory
for several components: StandaloneMmCpu, StandaloneMmCoreEntryPoint
and StandaloneMmMemLib. The source file is built for both 32b and 64b
Arm targets.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v2

Changes since v1:
- ARM_SMC_ID_MM_COMMUNICATE 32b/64b agnostic helper ID is defined
in ArmStdSmc.h (see 1st commit in this series) instead of being
local to EventHandle.c.
- Fix void occurrence to VOID.
- Fix path in StandaloneMmPkg/StandaloneMmPkg.dsc
---
StandaloneMmPkg/Core/StandaloneMmCore.inf | 2 +-
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/EventHandle.c | 5 +++--
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/StandaloneMmCpu.c | 2 +-
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/StandaloneMmCpu.h | 0
StandaloneMmPkg/Drivers/StandaloneMmCpu/{AArch64 => }/StandaloneMmCpu.inf | 0
StandaloneMmPkg/Include/Library/{AArch64 => Arm}/StandaloneMmCoreEntryPoint.h | 0
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/{AArch64 => Arm}/CreateHobList.c | 2 +-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/{AArch64 => Arm}/SetPermissions.c | 2 +-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/{AArch64 => Arm}/StandaloneMmCoreEntryPoint.c | 16 ++++++++--------
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 14 +++++++-------
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/{AArch64 => Arm}/StandaloneMmCoreHobLib.c | 0
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/{AArch64 => Arm}/StandaloneMmCoreHobLibInternal.c | 0
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf | 8 ++++----
StandaloneMmPkg/Library/StandaloneMmMemLib/{AArch64/StandaloneMmMemLibInternal.c => ArmStandaloneMmMemLibInternal.c} | 9 ++++++++-
StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf | 6 +++---
StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf | 2 +-
StandaloneMmPkg/StandaloneMmPkg.dsc | 10 +++++-----
17 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index 87bf6e9440..56042b7b39 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -17,7 +17,7 @@
PI_SPECIFICATION_VERSION = 0x00010032
ENTRY_POINT = StandaloneMmMain
-# VALID_ARCHITECTURES = IA32 X64 AARCH64
+# VALID_ARCHITECTURES = IA32 X64 AARCH64 ARM
[Sources]
StandaloneMmCore.c
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
similarity index 95%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 63fbe26642..165d696f99 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -2,6 +2,7 @@
Copyright (c) 2016 HP Development Company, L.P.
Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
+ Copyright (c) 2021, Linaro Limited
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -92,8 +93,8 @@ PiMmStandaloneArmTfCpuDriverEntry (
// receipt of a synchronous MM request. Use the Event ID to distinguish
// between synchronous and asynchronous events.
//
- if ((ARM_SMC_ID_MM_COMMUNICATE_AARCH64 != EventId) &&
- (ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64 != EventId)) {
+ if ((ARM_SMC_ID_MM_COMMUNICATE != EventId) &&
+ (ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ != EventId)) {
DEBUG ((DEBUG_INFO, "UnRecognized Event - 0x%x\n", EventId));
return EFI_INVALID_PARAMETER;
}
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
similarity index 96%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index d4590bcd19..10097f792f 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -10,7 +10,7 @@
#include <Base.h>
#include <Pi/PiMmCis.h>
-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <Library/DebugLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/ArmLib.h>
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
similarity index 100%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.h
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
similarity index 100%
rename from StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
rename to StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
diff --git a/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
similarity index 100%
rename from StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
rename to StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
similarity index 97%
rename from StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
index 4d4cf3d5ff..85f8194687 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
@@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MmramMemoryReserve.h>
#include <Guid/MpInformation.h>
-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <Library/ArmMmuLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/DebugLib.h>
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c
similarity index 96%
rename from StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c
index 4a380df4a6..cd4b90823e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c
@@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MmramMemoryReserve.h>
#include <Guid/MpInformation.h>
-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <Library/ArmMmuLib.h>
#include <Library/ArmSvcLib.h>
#include <Library/DebugLib.h>
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
similarity index 94%
rename from StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index b445d6942e..49cf51a789 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -10,7 +10,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <PiMm.h>
-#include <Library/AArch64/StandaloneMmCoreEntryPoint.h>
+#include <Library/Arm/StandaloneMmCoreEntryPoint.h>
#include <PiPei.h>
#include <Guid/MmramMemoryReserve.h>
@@ -182,13 +182,13 @@ DelegatedEventLoop (
}
if (FfaEnabled) {
- EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+ EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP;
EventCompleteSvcArgs->Arg1 = 0;
EventCompleteSvcArgs->Arg2 = 0;
- EventCompleteSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ EventCompleteSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE;
EventCompleteSvcArgs->Arg4 = SvcStatus;
} else {
- EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ EventCompleteSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE;
EventCompleteSvcArgs->Arg1 = SvcStatus;
}
}
@@ -273,13 +273,13 @@ InitArmSvcArgs (
)
{
if (FeaturePcdGet (PcdFfaEnable)) {
- InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64;
+ InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP;
InitMmFoundationSvcArgs->Arg1 = 0;
InitMmFoundationSvcArgs->Arg2 = 0;
- InitMmFoundationSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ InitMmFoundationSvcArgs->Arg3 = ARM_SVC_ID_SP_EVENT_COMPLETE;
InitMmFoundationSvcArgs->Arg4 = *Ret;
} else {
- InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64;
+ InitMmFoundationSvcArgs->Arg0 = ARM_SVC_ID_SP_EVENT_COMPLETE;
InitMmFoundationSvcArgs->Arg1 = *Ret;
}
}
@@ -395,7 +395,7 @@ _ModuleEntryPoint (
//
ProcessModuleEntryPointList (HobStart);
- DEBUG ((DEBUG_INFO, "Shared Cpu Driver EP 0x%lx\n", (UINT64) CpuDriverEntryPoint));
+ DEBUG ((DEBUG_INFO, "Shared Cpu Driver EP %p\n", (VOID *) CpuDriverEntryPoint));
finish:
if (Status == RETURN_UNSUPPORTED) {
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 4fa426f58e..1762586cfa 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -21,10 +21,10 @@
# VALID_ARCHITECTURES = IA32 X64 IPF EBC (EBC is for build only)
#
-[Sources.AARCH64]
- AArch64/StandaloneMmCoreEntryPoint.c
- AArch64/SetPermissions.c
- AArch64/CreateHobList.c
+[Sources.AARCH64, Sources.ARM]
+ Arm/StandaloneMmCoreEntryPoint.c
+ Arm/SetPermissions.c
+ Arm/CreateHobList.c
[Sources.X64]
X64/StandaloneMmCoreEntryPoint.c
@@ -34,14 +34,14 @@
MdeModulePkg/MdeModulePkg.dec
StandaloneMmPkg/StandaloneMmPkg.dec
-[Packages.AARCH64]
+[Packages.ARM, Packages.AARCH64]
ArmPkg/ArmPkg.dec
[LibraryClasses]
BaseLib
DebugLib
-[LibraryClasses.AARCH64]
+[LibraryClasses.ARM, LibraryClasses.AARCH64]
StandaloneMmMmuLib
ArmSvcLib
@@ -51,7 +51,7 @@
gEfiStandaloneMmNonSecureBufferGuid
gEfiArmTfCpuDriverEpDescriptorGuid
-[FeaturePcd.AARCH64]
+[FeaturePcd.ARM, FeaturePcd.AARCH64]
gArmTokenSpaceGuid.PcdFfaEnable
[BuildOptions]
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
similarity index 100%
rename from StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLib.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLibInternal.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLibInternal.c
similarity index 100%
rename from StandaloneMmPkg/Library/StandaloneMmCoreHobLib/AArch64/StandaloneMmCoreHobLibInternal.c
rename to StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLibInternal.c
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
index a2559920e8..34ed536480 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
@@ -22,7 +22,7 @@
LIBRARY_CLASS = HobLib|MM_CORE_STANDALONE
#
-# VALID_ARCHITECTURES = X64 AARCH64
+# VALID_ARCHITECTURES = X64 AARCH64 ARM
#
[Sources.common]
Common.c
@@ -30,9 +30,9 @@
[Sources.X64]
X64/StandaloneMmCoreHobLib.c
-[Sources.AARCH64]
- AArch64/StandaloneMmCoreHobLib.c
- AArch64/StandaloneMmCoreHobLibInternal.c
+[Sources.AARCH64, Sources.ARM]
+ Arm/StandaloneMmCoreHobLib.c
+ Arm/StandaloneMmCoreHobLibInternal.c
[Packages]
MdePkg/MdePkg.dec
diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c b/StandaloneMmPkg/Library/StandaloneMmMemLib/ArmStandaloneMmMemLibInternal.c
similarity index 86%
rename from StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
rename to StandaloneMmPkg/Library/StandaloneMmMemLib/ArmStandaloneMmMemLibInternal.c
index 4124959e04..fa7df46413 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/ArmStandaloneMmMemLibInternal.c
@@ -20,6 +20,13 @@
//
extern EFI_PHYSICAL_ADDRESS mMmMemLibInternalMaximumSupportAddress;
+#ifdef MDE_CPU_AARCH64
+#define ARM_PHYSICAL_ADDRESS_BITS 36
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_PHYSICAL_ADDRESS_BITS 32
+#endif
+
/**
Calculate and save the maximum support address.
@@ -31,7 +38,7 @@ MmMemLibInternalCalculateMaximumSupportAddress (
{
UINT8 PhysicalAddressBits;
- PhysicalAddressBits = 36;
+ PhysicalAddressBits = ARM_PHYSICAL_ADDRESS_BITS;
//
// Save the maximum support address in one global variable
diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
index 062b0d7a11..b29d97a746 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
@@ -28,7 +28,7 @@
#
# The following information is for reference only and not required by the build tools.
#
-# VALID_ARCHITECTURES = IA32 X64 AARCH64
+# VALID_ARCHITECTURES = IA32 X64 AARCH64 ARM
#
[Sources.Common]
@@ -37,8 +37,8 @@
[Sources.IA32, Sources.X64]
X86StandaloneMmMemLibInternal.c
-[Sources.AARCH64]
- AArch64/StandaloneMmMemLibInternal.c
+[Sources.AARCH64, Sources.ARM]
+ ArmStandaloneMmMemLibInternal.c
[Packages]
MdePkg/MdePkg.dec
diff --git a/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf b/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
index a2a059c5d6..ffb2a6d083 100644
--- a/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
+++ b/StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
@@ -20,7 +20,7 @@
#
# The following information is for reference only and not required by the build tools.
#
-# VALID_ARCHITECTURES = AARCH64
+# VALID_ARCHITECTURES = AARCH64|ARM
#
#
diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 0c45df95e2..772af1b72b 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -20,7 +20,7 @@
PLATFORM_VERSION = 1.0
DSC_SPECIFICATION = 0x00010011
OUTPUT_DIRECTORY = Build/StandaloneMm
- SUPPORTED_ARCHITECTURES = AARCH64|X64
+ SUPPORTED_ARCHITECTURES = AARCH64|X64|ARM
BUILD_TARGETS = DEBUG|RELEASE
SKUID_IDENTIFIER = DEFAULT
@@ -60,7 +60,7 @@
StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf
VariableMmDependency|StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
-[LibraryClasses.AARCH64]
+[LibraryClasses.AARCH64, LibraryClasses.ARM]
ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
@@ -118,8 +118,8 @@
StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
-[Components.AARCH64]
- StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf
+[Components.AARCH64, Components.ARM]
+ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
###################################################################################################
@@ -131,7 +131,7 @@
# module style (EDK or EDKII) specified in [Components] section.
#
###################################################################################################
-[BuildOptions.AARCH64]
+[BuildOptions.AARCH64, BuildOptions.ARM]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv8-a+nofp -mstrict-align
[SAMI] Looks like I missed this in my previous review, sorry.
Is '-march=armv8-axxx' correct here? or we need another ARM section with -march=armv7-a?
Can you also check if '-mstrict-align' is right and if '-fno-stack-protector' is needed instead, please?

I used the following setting from your edk2-platforms patch v1 4/4, file Platform/StandaloneMm/PlatformStandaloneMmPkg/PlatformStandaloneMmRpmb.dsc
and was able to build on a Windows host PC using gcc-arm-9.2-2019.12-mingw-w64-i686-arm-none-eabi compiler.
[BuildOptions.ARM]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv7-a
GCC:*_*_*_CC_FLAGS = -fno-stack-protector
[/SAMI]
GCC:*_*_*_CC_FLAGS = -mstrict-align
Re: [PATCH v3 4/5] StandaloneMmPkg: fix pointer/int casts against 32bit architectures

Sami Mujawar
 

Hi Etienne,

Thank you for this patch.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar


On 17/05/2021 08:40 AM, Etienne Carriere wrote:
Use intermediate (UINTN) cast when casting int from/to pointer. This
is needed as UINT64 values cast from/to 32bit pointer for 32bit
architectures.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v2
No change since v1
---
 StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c                       |  8 ++++----
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c              | 14 +++++++-------
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
index 6884095c49..d4590bcd19 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.c
@@ -164,8 +164,8 @@ StandaloneMmCpuInitialize (
 
   // Share the entry point of the CPU driver
   DEBUG ((DEBUG_INFO, "Sharing Cpu Driver EP *0x%lx = 0x%lx\n",
-          (UINT64) CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr,
-          (UINT64) PiMmStandaloneArmTfCpuDriverEntry));
+          (UINTN) CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr,
+          (UINTN) PiMmStandaloneArmTfCpuDriverEntry));
   *(CpuDriverEntryPointDesc->ArmTfCpuDriverEpPtr) = PiMmStandaloneArmTfCpuDriverEntry;
 
   // Find the descriptor that contains the whereabouts of the buffer for
@@ -180,8 +180,8 @@ StandaloneMmCpuInitialize (
     return Status;
   }
 
-  DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalStart - 0x%lx\n", (UINT64) NsCommBufMmramRange->PhysicalStart));
-  DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalSize - 0x%lx\n", (UINT64) NsCommBufMmramRange->PhysicalSize));
+  DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalStart - 0x%lx\n", (UINTN) NsCommBufMmramRange->PhysicalStart));
+  DEBUG ((DEBUG_INFO, "mNsCommBuffer.PhysicalSize - 0x%lx\n", (UINTN) NsCommBufMmramRange->PhysicalSize));
 
   CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
   DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
index e8fb96bd6e..4d4cf3d5ff 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/CreateHobList.c
@@ -72,14 +72,14 @@ CreateHobListFromBootInfo (
 
   // Create a hoblist with a PHIT and EOH
   HobStart = HobConstructor (
-               (VOID *) PayloadBootInfo->SpMemBase,
+               (VOID *) (UINTN) PayloadBootInfo->SpMemBase,
                (UINTN)  PayloadBootInfo->SpMemLimit - PayloadBootInfo->SpMemBase,
-               (VOID *) PayloadBootInfo->SpHeapBase,
-               (VOID *) (PayloadBootInfo->SpHeapBase + PayloadBootInfo->SpHeapSize)
+               (VOID *) (UINTN) PayloadBootInfo->SpHeapBase,
+               (VOID *) (UINTN) (PayloadBootInfo->SpHeapBase + PayloadBootInfo->SpHeapSize)
                );
 
   // Check that the Hoblist starts at the bottom of the Heap
-  ASSERT (HobStart == (VOID *) PayloadBootInfo->SpHeapBase);
+  ASSERT (HobStart == (VOID *) (UINTN) PayloadBootInfo->SpHeapBase);
 
   // Build a Boot Firmware Volume HOB
   BuildFvHob (PayloadBootInfo->SpImageBase, PayloadBootInfo->SpImageSize);
@@ -190,9 +190,9 @@ CreateHobListFromBootInfo (
   MmramRanges[3].RegionState   = EFI_CACHEABLE | EFI_ALLOCATED;
 
   // Base and size of heap memory shared by all cpus
-  MmramRanges[4].PhysicalStart = (EFI_PHYSICAL_ADDRESS) HobStart;
-  MmramRanges[4].CpuStart      = (EFI_PHYSICAL_ADDRESS) HobStart;
-  MmramRanges[4].PhysicalSize  = HobStart->EfiFreeMemoryBottom - (EFI_PHYSICAL_ADDRESS) HobStart;
+  MmramRanges[4].PhysicalStart = (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+  MmramRanges[4].CpuStart      = (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+  MmramRanges[4].PhysicalSize  = HobStart->EfiFreeMemoryBottom - (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
   MmramRanges[4].RegionState   = EFI_CACHEABLE | EFI_ALLOCATED;
 
   // Base and size of heap memory shared by all cpus
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 6c50f470aa..b445d6942e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -328,7 +328,7 @@ _ModuleEntryPoint (
 
   // Locate PE/COFF File information for the Standalone MM core module
   Status = LocateStandaloneMmCorePeCoffData (
-             (EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
+             (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) PayloadBootInfo->SpImageBase,
              &TeData,
              &TeDataSize
              );

Re: [PATCH v3 3/5] GenFv: Arm: support images entered in Thumb mode

Sami Mujawar
 

Hi Etienne,

Thank you for this patch.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar


On 17/05/2021 08:40 AM, Etienne Carriere wrote:
Change GenFv for Arm architecture to generate a specific jump
instruction as image entry instruction, when the target entry label
is assembled with Thumb instruction set. This is possible since
SecCoreEntryAddress value fetched from the PE32 has its LSBit set when
the entry instruction executes in Thumb mode.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
Changes since v2:
- Fix missing parentheses in expression.

Changes since v1:
- Fix typos in commit log and inline comments
- Change if() test operand to be an explicit boolean
---
 BaseTools/Source/C/GenFv/GenFvInternalLib.c | 38 +++++++++++++++-----
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/BaseTools/Source/C/GenFv/GenFvInternalLib.c b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
index 6e296b8ad6..6cf9c84e73 100644
--- a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
+++ b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
@@ -34,9 +34,27 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "FvLib.h"
 #include "PeCoffLib.h"
 
-#define ARMT_UNCONDITIONAL_JUMP_INSTRUCTION       0xEB000000
 #define ARM64_UNCONDITIONAL_JUMP_INSTRUCTION      0x14000000
 
+/*
+ * Arm instruction to jump to Fv entry instruction in Arm or Thumb mode.
+ * From ARM Arch Ref Manual versions b/c/d, section A8.8.25 BL, BLX (immediate)
+ * BLX (encoding A2) branches to offset in Thumb instruction set mode.
+ * BL (encoding A1) branches to offset in Arm instruction set mode.
+ */
+#define ARM_JUMP_OFFSET_MAX        0xffffff
+#define ARM_JUMP_TO_ARM(Offset)    (0xeb000000 | ((Offset - 8) >> 2))
+
+#define _ARM_JUMP_TO_THUMB(Imm32)  (0xfa000000 | \
+                                    (((Imm32) & (1 << 1)) << (24 - 1)) | \
+                                    (((Imm32) >> 2) & 0x7fffff))
+#define ARM_JUMP_TO_THUMB(Offset)  _ARM_JUMP_TO_THUMB((Offset) - 8)
+
+/*
+ * Arm instruction to retrun from exception (MOVS PC, LR)
+ */
+#define ARM_RETURN_FROM_EXCEPTION  0xE1B0F07E
+
 BOOLEAN mArm = FALSE;
 BOOLEAN mRiscV = FALSE;
 STATIC UINT32   MaxFfsAlignment = 0;
@@ -2203,23 +2221,25 @@ Returns:
     // if we found an SEC core entry point then generate a branch instruction
     // to it and populate a debugger SWI entry as well
     if (UpdateVectorSec) {
+      UINT32                    EntryOffset;
 
       VerboseMsg("UpdateArmResetVectorIfNeeded updating ARM SEC vector");
 
-      // B SecEntryPoint - signed_immed_24 part +/-32MB offset
-      // on ARM, the PC is always 8 ahead, so we're not really jumping from the base address, but from base address + 8
-      ResetVector[0] = (INT32)(SecCoreEntryAddress - FvInfo->BaseAddress - 8) >> 2;
+      EntryOffset = (INT32)(SecCoreEntryAddress - FvInfo->BaseAddress);
 
-      if (ResetVector[0] > 0x00FFFFFF) {
-        Error(NULL, 0, 3000, "Invalid", "SEC Entry point must be within 32MB of the start of the FV");
+      if (EntryOffset > ARM_JUMP_OFFSET_MAX) {
+          Error(NULL, 0, 3000, "Invalid", "SEC Entry point offset above 1MB of the start of the FV");
         return EFI_ABORTED;
       }
 
-      // Add opcode for an unconditional branch with no link. i.e.: " B SecEntryPoint"
-      ResetVector[0] |= ARMT_UNCONDITIONAL_JUMP_INSTRUCTION;
+      if ((SecCoreEntryAddress & 1) != 0) {
+        ResetVector[0] = ARM_JUMP_TO_THUMB(EntryOffset);
+      } else {
+        ResetVector[0] = ARM_JUMP_TO_ARM(EntryOffset);
+      }
 
       // SWI handler movs   pc,lr. Just in case a debugger uses SWI
-      ResetVector[2] = 0xE1B0F07E;
+      ResetVector[2] = ARM_RETURN_FROM_EXCEPTION;
 
       // Place holder to support a common interrupt handler from ROM.
       // Currently not supported. For this to be used the reset vector would not be in this FV

Re: [PATCH v3 2/5] ArmPkg: prepare 32bit ARM build of StandaloneMmPkg

Sami Mujawar
 

Hi Etienne,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar


On 17/05/2021 08:40 AM, Etienne Carriere wrote:
Changes in ArmPkg to prepare building StandaloneMm firmware for
32bit Arm architectures.

Adds MmCommunicationDxe driver and ArmMmuPeiLib and
ArmmmuStandaloneMmLib libraries to the list of the standard
components build for ArmPkg on when ARM architectures.

Changes path of source file AArch64/ArmMmuStandaloneMmLib.c
and compile it for both 32bit and 64bit architectures.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v2
No change since v1
---
 ArmPkg/ArmPkg.dec                                                       |  2 +-
 ArmPkg/ArmPkg.dsc                                                       |  2 +-
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c                     |  2 +-
 ArmPkg/Library/StandaloneMmMmuLib/{AArch64 => }/ArmMmuStandaloneMmLib.c | 15 ++++++++-------
 ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf             |  6 +++---
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 214b2f5892..6ed51edd03 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -137,7 +137,7 @@
   # hardware coherency (i.e., no virtualization or cache coherent DMA)
   gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride|FALSE|BOOLEAN|0x00000043
 
-[PcdsFeatureFlag.AARCH64]
+[PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.ARM]
   ## Used to select method for requesting services from S-EL1.<BR><BR>
   #   TRUE  - Selects FF-A calls for communication between S-EL0 and SPMC.<BR>
   #   FALSE - Selects SVC calls for communication between S-EL0 and SPMC.<BR>
diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 926986cf7f..4c79dadf9e 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -158,7 +158,7 @@
   ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
   ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf
 
-[Components.AARCH64]
+[Components.AARCH64, Components.ARM]
   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
   ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
   ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index b1e3095809..4ae38a9f22 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -125,7 +125,7 @@ MmCommunication2Communicate (
   }
 
   // SMC Function ID
-  CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
+  CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE;
 
   // Cookie
   CommunicateSmcArgs.Arg1 = 0;
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
similarity index 92%
rename from ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
rename to ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
index dd014beec8..20f873e680 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
@@ -2,6 +2,7 @@
   File managing the MMU for ARMv8 architecture in S-EL0
 
   Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>
+  Copyright (c) 2021, Linaro Limited
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
@@ -62,7 +63,7 @@ SendMemoryPermissionRequest (
     // for other Direct Request calls which are not atomic
     // We therefore check only for Direct Response by the
     // callee.
-    if (SvcArgs->Arg0 == ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
+    if (SvcArgs->Arg0 == ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP) {
       // A Direct Response means FF-A success
       // Now check the payload for errors
       // The callee sends back the return value
@@ -164,13 +165,13 @@ GetMemoryPermissions (
   ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));
   if (FeaturePcdGet (PcdFfaEnable)) {
     // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
-    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
     SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
     SvcArgs.Arg2 = 0;
-    SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES;
     SvcArgs.Arg4 = BaseAddress;
   } else {
-    SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES;
     SvcArgs.Arg1 = BaseAddress;
     SvcArgs.Arg2 = 0;
     SvcArgs.Arg3 = 0;
@@ -219,15 +220,15 @@ RequestMemoryPermissionChange (
   ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));
   if (FeaturePcdGet (PcdFfaEnable)) {
     // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
-    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
     SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
     SvcArgs.Arg2 = 0;
-    SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES;
     SvcArgs.Arg4 = BaseAddress;
     SvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length);
     SvcArgs.Arg6 = Permissions;
   } else {
-    SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES;
     SvcArgs.Arg1 = BaseAddress;
     SvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length);
     SvcArgs.Arg3 = Permissions;
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
index 6c71fe0023..ff20e58980 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
@@ -16,14 +16,14 @@
   LIBRARY_CLASS                  = StandaloneMmMmuLib
   PI_SPECIFICATION_VERSION       = 0x00010032
 
-[Sources.AARCH64]
-  AArch64/ArmMmuStandaloneMmLib.c
+[Sources]
+  ArmMmuStandaloneMmLib.c
 
 [Packages]
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
 
-[FeaturePcd.AARCH64]
+[FeaturePcd.ARM, FeaturePcd.AARCH64]
   gArmTokenSpaceGuid.PcdFfaEnable
 
 [LibraryClasses]

Re: [PATCH v3 1/5] ArmPkg/IndustryStandard: 32b/64b agnostic FF-A, Mm SVC and Std SMC IDs

Sami Mujawar
 

Hi Etienne,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

On 17/05/2021 08:40 AM, Etienne Carriere wrote:
Defines ARM_SVC_ID_FFA_* and ARM_SVC_ID_SP_* identifiers for 32bit
function IDs as per SMCCC specification. Defines also generic ARM
SVC identifier macros to wrap 32bit or 64bit identifiers upon target
built architecture.

Cc: Achin Gupta <achin.gupta@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sughosh Ganu <sughosh.ganu@...>
Signed-off-by: Etienne Carriere <etienne.carriere@...>
---
No change since v2

Changes since v1:
- Define ARM_SMC_ID_MM_COMMUNICATE 32b/64b agnostic helper ID in
ArmStdSmc.h, as expected by few following commits in this series.
---
ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 12 ++++++++++++
ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 15 +++++++++++++++
ArmPkg/Include/IndustryStandard/ArmStdSmc.h | 8 ++++++++
3 files changed, 35 insertions(+)

diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index 65b8343ade..ebcb54b28b 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -17,9 +17,21 @@
#define ARM_FFA_SVC_H_
#define ARM_SVC_ID_FFA_VERSION_AARCH32 0x84000063
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH32 0x8400006F
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH32 0x84000070
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64 0xC400006F
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64 0xC4000070
+/* Generic IDs when using AArch32 or AArch64 execution state */
+#ifdef MDE_CPU_AARCH64
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH32
+#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH32
+#endif
+
#define SPM_MAJOR_VERSION_FFA 1
#define SPM_MINOR_VERSION_FFA 0
diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 33d60ccf17..deb3bc99d2 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -15,10 +15,25 @@
* privileged operations on its behalf.
*/
#define ARM_SVC_ID_SPM_VERSION_AARCH32 0x84000060
+#define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH32 0x84000061
+#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH32 0x84000064
+#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH32 0x84000065
#define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64 0xC4000061
#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64 0xC4000064
#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64 0xC4000065
+/* Generic IDs when using AArch32 or AArch64 execution state */
+#ifdef MDE_CPU_AARCH64
+#define ARM_SVC_ID_SP_EVENT_COMPLETE ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64
+#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64
+#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_SVC_ID_SP_EVENT_COMPLETE ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH32
+#define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH32
+#define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH32
+#endif
+
#define SET_MEM_ATTR_DATA_PERM_MASK 0x3
#define SET_MEM_ATTR_DATA_PERM_SHIFT 0
#define SET_MEM_ATTR_DATA_PERM_NO_ACCESS 0
diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
index 67afb0ea2d..9116a291da 100644
--- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
@@ -49,6 +49,14 @@
#define ARM_SMC_ID_MM_COMMUNICATE_AARCH32 0x84000041
#define ARM_SMC_ID_MM_COMMUNICATE_AARCH64 0xC4000041
+/* Generic ID when using AArch32 or AArch64 execution state */
+#ifdef MDE_CPU_AARCH64
+#define ARM_SMC_ID_MM_COMMUNICATE ARM_SMC_ID_MM_COMMUNICATE_AARCH64
+#endif
+#ifdef MDE_CPU_ARM
+#define ARM_SMC_ID_MM_COMMUNICATE ARM_SMC_ID_MM_COMMUNICATE_AARCH32
+#endif
+
/* MM return error codes */
#define ARM_SMC_MM_RET_SUCCESS 0
#define ARM_SMC_MM_RET_NOT_SUPPORTED -1
Re: [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix possible uninitialized use

Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sergei
Dmitrouk
Sent: Tuesday, May 18, 2021 12:23 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>
Subject: Re: [edk2-devel] [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix
possible uninitialized use

If the function gets invalid value for the `ResizableBarOp` parameter and
asserts are disabled, `Bit` can be used uninitialized.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Sergei Dmitrouk <sergei@...>
---

Notes:
v2:
- simplify if-statement to avoid unused branches

Hello,

Since the V1 is a patch series, I would suggest to send the whole series for V2 changes (even if other patches are unchanged).

With this handled:
Reviewed-by: Hao A Wu <hao.a.wu@...>

Best Regards,
Hao Wu


MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 6bba28367165..4caac56f1dcd 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1778,10 +1778,9 @@ PciProgramResizableBar (

if (ResizableBarOp == PciResizableBarMax) {
Bit = HighBitSet64(Capabilities);
- } else if (ResizableBarOp == PciResizableBarMin) {
+ } else {
+ ASSERT (ResizableBarOp == PciResizableBarMin);
Bit = LowBitSet64(Capabilities);
- } else {
- ASSERT ((ResizableBarOp == PciResizableBarMax) || (ResizableBarOp ==
PciResizableBarMin));
}

ASSERT (Bit >= 0);
--
2.17.6
Re: [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation

Ni, Ray
 

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Sunday, May 16, 2021 9:39 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation

On 05/15/21 02:04, Ni, Ray wrote:
Laszlo,
Do you think that another API is also needed: GetPhysicalAddressWidth() that returns number 36/52?
No. The GetPhysicalAddressBits() function that I proposed already returns this information. It has three outputs: the number of
bits (that is, the width), as return value, and the two optional output parameters.

So if you only need the the bit count, call

GetPhysicalAddressBits (NULL, NULL);

These calculations are so cheap and small that keeping them in a single function makes a lot of sense in my opinion.
I wasn't aware of the return value of the API. with your API, there is no need for another API to retrieve the address size.

For a critical bugfix, I would prefer not mixing the actual fix with the introduction of the symbolic names. Your patch currently
fixes three things at the same time: (1) coding style (it replaces magic constants with macros / type names), (2) a bug in
calculation, (3) a missing CPUID "maximum function" check.

Maybe writing a separate patch for each of these is unjustified, but I was really unhappy to see that the commit message said
nothing about (1) and (3), and I had to hunt down (2) between the other changes.

The minimal fix -- that is, the fix for (2) -- would be just one line:

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index fd6583f9d172..4592b76fe595 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1920,7 +1920,7 @@ InitializeMpServiceData (
//
AsmCpuid (0x80000008, (UINT32*)&Index, NULL, NULL, NULL);
gPhyMask = LShiftU64 (1, (UINT8)Index) - 1;
- gPhyMask &= (1ull << 48) - EFI_PAGE_SIZE;
+ gPhyMask &= 0xfffffffffffff000ULL;

//
// Create page tables


I don't like that the patch currently does three things but only documents one.
Thanks for explaining why you don't think it's a good patch. I thought anytime changing a code,
I should try to make it better, functional better, looks better.

I will follow your suggestion next time for bug fixes.


That said, if you are out of time, feel free to go ahead with Eric's R-b.
Indeed. thanks for the understanding.


Thanks
Laszlo
15661 - 15680 of 90847