[PATCH 0/5] StandaloneMmPkg: make StMM core relocatable

[PATCH 0/5] StandaloneMmPkg: make StMM core relocatable

[Ard Biesheuvel]

It is not always possible to deploy the standalone MM core in a way where
the runtime address is known at build time. This does not matter for most
modules, since they are relocated at dispatch time. However, for the MM
core itself, it means we need to do some extra work to relocate the image
in place if it ends up at a different offset than expected.

On AARCH64, the standalone MM stack is deployed inside a non-privileged
secure world container which only has limited control over its memory
mappings, and so we need to ensure that the executable code itself is
free of absolute quantities that need to be fixed up. This is very similar
to how shared libraries are constructed, given that pages can only be
shared between processes if they are not modified, even by the dynamic
loader. So we can use this support to emit the standaline MM core in a
way that guarantees that the executable code does not need to modify
itself (patch #4)

Patch #5 adds the actual code to perform the self relocation after the
.data section has been made writable and non-executable. Note that the
PE/COFF library code modifies the header in place, and so in the case
where we need to perform the runtime relocation, we need to remap the
header page writable and non-executable as well.

The remaining patches are optimizations and fixes I picked up along
the way.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Ard Biesheuvel (5):
  MdePkg/BasePrintLib: avoid absolute addresses for error strings
  StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string
  StandaloneMmPkg/Core: add missing GUID reference
  StandaloneMmPkg: generate position independent code for StMM core
  StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the
    fly

 StandaloneMmPkg/Core/StandaloneMmCore.inf                                               |  5 +++++
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |  3 +++
 StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h                    |  2 ++
 MdePkg/Library/BasePrintLib/PrintLibInternal.c                                          |  2 +-
 StandaloneMmPkg/Core/Dispatcher.c                                                       |  2 +-
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c             | 11 +++++++---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 22 ++++++++++++++++++++
 7 files changed, 42 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings

[Ard Biesheuvel]

The mStatusString[] is constructed as an array of pointer-to-char, which
means that on X64 or AARCH64, it is emitted as a single linear list of
64-bit quantities, each containing the absolute address of one of the
string literals in memory.

This means that each string takes up 8 bytes of additional space, along
with 2 bytes of relocation data. It also means that the array cannot be
used until PE/COFF relocation has completed, and so the following
invocation

    Status = PeCoffLoaderRelocateImage (&ImageContext);
    ASSERT_EFI_ERROR (Status);

that we will be introducing into StandaloneMmCore entrypoint for AARCH64
to relocate the executable on the fly is guaranteed to return bogus output
or crash, which is less than helpful.

So fix both issues, by emitting mStatusString as an array of char arrays
instead. The memory footprint increases from 955 to 975 bytes, but given
that in the latter case, the overhead consists of 410 NUL characters
rather than 390 bytes worth of absolute addresses and relocation records,
the impact on a compressed image is actually positive. For example, when
building ArmVirtQemu.dsc in RELEASE mode for AARCH64 with the GCC5 profile,
I get:

  Before

  FV Space Information
  FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free
  FVMAIN_COMPACT [38%Full] 2093056 total, 811560 used, 1281496 free

  After

  FV Space Information
  FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free
  FVMAIN_COMPACT [38%Full] 2093056 total, 813488 used, 1279568 free

So the compressed image is 4 KB smaller, whereas the entire image is
< 2 KB larger, which is in the order of 0.2 %

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index b6ec5ac4fbb9..c8b932c7e07a 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -27,7 +27,7 @@
 
 GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};
 
-GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST mStatusString[] = {
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mStatusString[][25] = {
   "Success",                      //  RETURN_SUCCESS                = 0
   "Warning Unknown Glyph",        //  RETURN_WARN_UNKNOWN_GLYPH     = 1
   "Warning Delete Failure",       //  RETURN_WARN_DELETE_FAILURE    = 2
-- 
2.26.2

Re: [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings

[Ard Biesheuvel]

On 6/10/20 10:17 AM, Ard Biesheuvel wrote:
> The mStatusString[] is constructed as an array of pointer-to-char, which
> means that on X64 or AARCH64, it is emitted as a single linear list of
> 64-bit quantities, each containing the absolute address of one of the
> string literals in memory.
> 
> This means that each string takes up 8 bytes of additional space, along
> with 2 bytes of relocation data. It also means that the array cannot be
> used until PE/COFF relocation has completed, and so the following
> invocation
> 
>      Status = PeCoffLoaderRelocateImage (&ImageContext);
>      ASSERT_EFI_ERROR (Status);
> 
> that we will be introducing into StandaloneMmCore entrypoint for AARCH64
> to relocate the executable on the fly is guaranteed to return bogus output
> or crash, which is less than helpful.
> 
> So fix both issues, by emitting mStatusString as an array of char arrays
> instead. The memory footprint increases from 955 to 975 bytes, but given
> that in the latter case, the overhead consists of 410 NUL characters
> rather than 390 bytes worth of absolute addresses and relocation records,
> the impact on a compressed image is actually positive. For example, when
> building ArmVirtQemu.dsc in RELEASE mode for AARCH64 with the GCC5 profile,
> I get:
> 
>    Before
> 
>    FV Space Information
>    FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free
>    FVMAIN_COMPACT [38%Full] 2093056 total, 811560 used, 1281496 free
> 
>    After
> 
>    FV Space Information
>    FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free
>    FVMAIN_COMPACT [38%Full] 2093056 total, 813488 used, 1279568 free
> 
> So the compressed image is 4 KB smaller, whereas the entire image is
> < 2 KB larger, which is in the order of 0.2 %
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Actually, I found a slightly better way of doing this, by applying the 
following delta on top, i.e., split the errors and warnings, and use an 
array of 21 byte character arrays for the former.

My build went from

FV Space Information

FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free
FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used, 1300072 free

to

FV Space Information
FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free
FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used, 1301984 free


in this case, i.e., both compressed and non-compressed images are 
slightly smaller.



--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -27,13 +27,16 @@

  GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = 
{'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};

-GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mStatusString[][25] = {
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mWarningString[][25] = {
    "Success",                      //  RETURN_SUCCESS                = 0
    "Warning Unknown Glyph",        //  RETURN_WARN_UNKNOWN_GLYPH     = 1
    "Warning Delete Failure",       //  RETURN_WARN_DELETE_FAILURE    = 2
    "Warning Write Failure",        //  RETURN_WARN_WRITE_FAILURE     = 3
    "Warning Buffer Too Small",     //  RETURN_WARN_BUFFER_TOO_SMALL  = 4
    "Warning Stale Data",           //  RETURN_WARN_STALE_DATA        = 5
+};
+
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mErrorString[][21] = {
    "Load Error",                   //  RETURN_LOAD_ERROR             = 
1  | MAX_BIT
    "Invalid Parameter",            //  RETURN_INVALID_PARAMETER      = 
2  | MAX_BIT
    "Unsupported",                  //  RETURN_UNSUPPORTED            = 
3  | MAX_BIT
@@ -996,12 +999,12 @@ BasePrintLibSPrintMarker (
            //
            Index = Status & ~MAX_BIT;
            if (Index > 0 && Index <= ERROR_STATUS_NUMBER) {
-            ArgumentString = mStatusString [Index + WARNING_STATUS_NUMBER];
+            ArgumentString = mErrorString [Index - 1];
            }
          } else {
            Index = Status;
            if (Index <= WARNING_STATUS_NUMBER) {
-            ArgumentString = mStatusString [Index];
+            ArgumentString = mWarningString [Index];
            }
          }
          if (ArgumentString == ValueBuffer) {


> ---
>   MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> index b6ec5ac4fbb9..c8b932c7e07a 100644
> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> @@ -27,7 +27,7 @@
>   
>   GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};
>   
> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST mStatusString[] = {
> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mStatusString[][25] = {
>     "Success",                      //  RETURN_SUCCESS                = 0
>     "Warning Unknown Glyph",        //  RETURN_WARN_UNKNOWN_GLYPH     = 1
>     "Warning Delete Failure",       //  RETURN_WARN_DELETE_FAILURE    = 2
> 

Re: [edk2-devel] [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings

[Michael D Kinney]

Hi Ard,

The size reduction is very interesting.  Would be good to 
see uncompressed size differences as well because this lib
is also used in pre-memory XIP modules that are fixed up
by the build tools that handle the relocation fixups.

The more general concern I have is if there are standard
C coding practices (such as a pre-initialized array of
pointers to strings) that are not compatible with
StandaloneMmPkg use cases.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Ard Biesheuvel
> Sent: Wednesday, June 10, 2020 1:37 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Sami Mujawar
> <sami.mujawar@arm.com>; Ilias Apalodimas
> <ilias.apalodimas@linaro.org>
> Subject: Re: [edk2-devel] [PATCH 1/5]
> MdePkg/BasePrintLib: avoid absolute addresses for error
> strings
> 
> On 6/10/20 10:17 AM, Ard Biesheuvel wrote:
> > The mStatusString[] is constructed as an array of
> pointer-to-char, which
> > means that on X64 or AARCH64, it is emitted as a
> single linear list of
> > 64-bit quantities, each containing the absolute
> address of one of the
> > string literals in memory.
> >
> > This means that each string takes up 8 bytes of
> additional space, along
> > with 2 bytes of relocation data. It also means that
> the array cannot be
> > used until PE/COFF relocation has completed, and so
> the following
> > invocation
> >
> >      Status = PeCoffLoaderRelocateImage
> (&ImageContext);
> >      ASSERT_EFI_ERROR (Status);
> >
> > that we will be introducing into StandaloneMmCore
> entrypoint for AARCH64
> > to relocate the executable on the fly is guaranteed to
> return bogus output
> > or crash, which is less than helpful.
> >
> > So fix both issues, by emitting mStatusString as an
> array of char arrays
> > instead. The memory footprint increases from 955 to
> 975 bytes, but given
> > that in the latter case, the overhead consists of 410
> NUL characters
> > rather than 390 bytes worth of absolute addresses and
> relocation records,
> > the impact on a compressed image is actually positive.
> For example, when
> > building ArmVirtQemu.dsc in RELEASE mode for AARCH64
> with the GCC5 profile,
> > I get:
> >
> >    Before
> >
> >    FV Space Information
> >    FVMAIN [99%Full] 4784768 total, 4784720 used, 48
> free
> >    FVMAIN_COMPACT [38%Full] 2093056 total, 811560
> used, 1281496 free
> >
> >    After
> >
> >    FV Space Information
> >    FVMAIN [99%Full] 4780672 total, 4780624 used, 48
> free
> >    FVMAIN_COMPACT [38%Full] 2093056 total, 813488
> used, 1279568 free
> >
> > So the compressed image is 4 KB smaller, whereas the
> entire image is
> > < 2 KB larger, which is in the order of 0.2 %
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Actually, I found a slightly better way of doing this,
> by applying the
> following delta on top, i.e., split the errors and
> warnings, and use an
> array of 21 byte character arrays for the former.
> 
> My build went from
> 
> FV Space Information
> 
> FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free
> FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used,
> 1300072 free
> 
> to
> 
> FV Space Information
> FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free
> FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used,
> 1301984 free
> 
> 
> in this case, i.e., both compressed and non-compressed
> images are
> slightly smaller.
> 
> 
> 
> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> @@ -27,13 +27,16 @@
> 
>   GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] =
> {'0','1','2','3','4','5','6','7','8','9','A','B','C','D'
> ,'E','F'};
> 
> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
> mStatusString[][25] = {
> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
> mWarningString[][25] = {
>     "Success",                      //  RETURN_SUCCESS
> = 0
>     "Warning Unknown Glyph",        //
> RETURN_WARN_UNKNOWN_GLYPH     = 1
>     "Warning Delete Failure",       //
> RETURN_WARN_DELETE_FAILURE    = 2
>     "Warning Write Failure",        //
> RETURN_WARN_WRITE_FAILURE     = 3
>     "Warning Buffer Too Small",     //
> RETURN_WARN_BUFFER_TOO_SMALL  = 4
>     "Warning Stale Data",           //
> RETURN_WARN_STALE_DATA        = 5
> +};
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
> mErrorString[][21] = {
>     "Load Error",                   //
> RETURN_LOAD_ERROR             =
> 1  | MAX_BIT
>     "Invalid Parameter",            //
> RETURN_INVALID_PARAMETER      =
> 2  | MAX_BIT
>     "Unsupported",                  //
> RETURN_UNSUPPORTED            =
> 3  | MAX_BIT
> @@ -996,12 +999,12 @@ BasePrintLibSPrintMarker (
>             //
>             Index = Status & ~MAX_BIT;
>             if (Index > 0 && Index <=
> ERROR_STATUS_NUMBER) {
> -            ArgumentString = mStatusString [Index +
> WARNING_STATUS_NUMBER];
> +            ArgumentString = mErrorString [Index - 1];
>             }
>           } else {
>             Index = Status;
>             if (Index <= WARNING_STATUS_NUMBER) {
> -            ArgumentString = mStatusString [Index];
> +            ArgumentString = mWarningString [Index];
>             }
>           }
>           if (ArgumentString == ValueBuffer) {
> 
> 
> > ---
> >   MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2
> +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git
> a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > index b6ec5ac4fbb9..c8b932c7e07a 100644
> > --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > @@ -27,7 +27,7 @@
> >
> >   GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[]
> =
> {'0','1','2','3','4','5','6','7','8','9','A','B','C','D'
> ,'E','F'};
> >
> > -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST
> mStatusString[] = {
> > +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
> mStatusString[][25] = {
> >     "Success",                      //  RETURN_SUCCESS
> = 0
> >     "Warning Unknown Glyph",        //
> RETURN_WARN_UNKNOWN_GLYPH     = 1
> >     "Warning Delete Failure",       //
> RETURN_WARN_DELETE_FAILURE    = 2
> >
> 
> 
> 

Re: [edk2-devel] [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings

[Ard Biesheuvel]

On 6/10/20 5:09 PM, Michael D Kinney via groups.io wrote:
> Hi Ard,
> 
> The size reduction is very interesting.  Would be good to
> see uncompressed size differences as well because this lib
> is also used in pre-memory XIP modules that are fixed up
> by the build tools that handle the relocation fixups.
> 

As I said in my reply-to-self, this results in a net decrease in both cases:

 >> FV Space Information
 >>
 >> FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free
 >> FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used,
 >> 1300072 free
 >>
 >> to
 >>
 >> FV Space Information
 >> FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free
 >> FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used,
 >> 1301984 free
 >>

FVMAIN is the uncompressed primary FV

FVMAIN_COMPACT contains FVMAIN in compressed form, along with the PEI 
and SEC modules.

So the XIP case is a win as well.

> The more general concern I have is if there are standard
> C coding practices (such as a pre-initialized array of
> pointers to strings) that are not compatible with
> StandaloneMmPkg use cases.
> 

In general, yes. In this case, it is only about being able to make it to 
the PE/COFF relocation routines and back, which happens very early (in a 
AARCH64 specific StMM core entrypoint library, mind you). So using 
statically initialized pointers is fine in general, just not in the 
early startup code that runs before relocation.




>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On
>> Behalf Of Ard Biesheuvel
>> Sent: Wednesday, June 10, 2020 1:37 AM
>> To: devel@edk2.groups.io
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
>> Liming <liming.gao@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Sami Mujawar
>> <sami.mujawar@arm.com>; Ilias Apalodimas
>> <ilias.apalodimas@linaro.org>
>> Subject: Re: [edk2-devel] [PATCH 1/5]
>> MdePkg/BasePrintLib: avoid absolute addresses for error
>> strings
>>
>> On 6/10/20 10:17 AM, Ard Biesheuvel wrote:
>>> The mStatusString[] is constructed as an array of
>> pointer-to-char, which
>>> means that on X64 or AARCH64, it is emitted as a
>> single linear list of
>>> 64-bit quantities, each containing the absolute
>> address of one of the
>>> string literals in memory.
>>>
>>> This means that each string takes up 8 bytes of
>> additional space, along
>>> with 2 bytes of relocation data. It also means that
>> the array cannot be
>>> used until PE/COFF relocation has completed, and so
>> the following
>>> invocation
>>>
>>>       Status = PeCoffLoaderRelocateImage
>> (&ImageContext);
>>>       ASSERT_EFI_ERROR (Status);
>>>
>>> that we will be introducing into StandaloneMmCore
>> entrypoint for AARCH64
>>> to relocate the executable on the fly is guaranteed to
>> return bogus output
>>> or crash, which is less than helpful.
>>>
>>> So fix both issues, by emitting mStatusString as an
>> array of char arrays
>>> instead. The memory footprint increases from 955 to
>> 975 bytes, but given
>>> that in the latter case, the overhead consists of 410
>> NUL characters
>>> rather than 390 bytes worth of absolute addresses and
>> relocation records,
>>> the impact on a compressed image is actually positive.
>> For example, when
>>> building ArmVirtQemu.dsc in RELEASE mode for AARCH64
>> with the GCC5 profile,
>>> I get:
>>>
>>>     Before
>>>
>>>     FV Space Information
>>>     FVMAIN [99%Full] 4784768 total, 4784720 used, 48
>> free
>>>     FVMAIN_COMPACT [38%Full] 2093056 total, 811560
>> used, 1281496 free
>>>
>>>     After
>>>
>>>     FV Space Information
>>>     FVMAIN [99%Full] 4780672 total, 4780624 used, 48
>> free
>>>     FVMAIN_COMPACT [38%Full] 2093056 total, 813488
>> used, 1279568 free
>>>
>>> So the compressed image is 4 KB smaller, whereas the
>> entire image is
>>> < 2 KB larger, which is in the order of 0.2 %
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>
>> Actually, I found a slightly better way of doing this,
>> by applying the
>> following delta on top, i.e., split the errors and
>> warnings, and use an
>> array of 21 byte character arrays for the former.
>>
>> My build went from
>>
>> FV Space Information
>>
>> FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free
>> FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used,
>> 1300072 free
>>
>> to
>>
>> FV Space Information
>> FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free
>> FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used,
>> 1301984 free
>>
>>
>> in this case, i.e., both compressed and non-compressed
>> images are
>> slightly smaller.
>>
>>
>>
>> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>> @@ -27,13 +27,16 @@
>>
>>    GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] =
>> {'0','1','2','3','4','5','6','7','8','9','A','B','C','D'
>> ,'E','F'};
>>
>> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
>> mStatusString[][25] = {
>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
>> mWarningString[][25] = {
>>      "Success",                      //  RETURN_SUCCESS
>> = 0
>>      "Warning Unknown Glyph",        //
>> RETURN_WARN_UNKNOWN_GLYPH     = 1
>>      "Warning Delete Failure",       //
>> RETURN_WARN_DELETE_FAILURE    = 2
>>      "Warning Write Failure",        //
>> RETURN_WARN_WRITE_FAILURE     = 3
>>      "Warning Buffer Too Small",     //
>> RETURN_WARN_BUFFER_TOO_SMALL  = 4
>>      "Warning Stale Data",           //
>> RETURN_WARN_STALE_DATA        = 5
>> +};
>> +
>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
>> mErrorString[][21] = {
>>      "Load Error",                   //
>> RETURN_LOAD_ERROR             =
>> 1  | MAX_BIT
>>      "Invalid Parameter",            //
>> RETURN_INVALID_PARAMETER      =
>> 2  | MAX_BIT
>>      "Unsupported",                  //
>> RETURN_UNSUPPORTED            =
>> 3  | MAX_BIT
>> @@ -996,12 +999,12 @@ BasePrintLibSPrintMarker (
>>              //
>>              Index = Status & ~MAX_BIT;
>>              if (Index > 0 && Index <=
>> ERROR_STATUS_NUMBER) {
>> -            ArgumentString = mStatusString [Index +
>> WARNING_STATUS_NUMBER];
>> +            ArgumentString = mErrorString [Index - 1];
>>              }
>>            } else {
>>              Index = Status;
>>              if (Index <= WARNING_STATUS_NUMBER) {
>> -            ArgumentString = mStatusString [Index];
>> +            ArgumentString = mWarningString [Index];
>>              }
>>            }
>>            if (ArgumentString == ValueBuffer) {
>>
>>
>>> ---
>>>    MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2
>> +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git
>> a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>> b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>>> index b6ec5ac4fbb9..c8b932c7e07a 100644
>>> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>>> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>>> @@ -27,7 +27,7 @@
>>>
>>>    GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[]
>> =
>> {'0','1','2','3','4','5','6','7','8','9','A','B','C','D'
>> ,'E','F'};
>>>
>>> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST
>> mStatusString[] = {
>>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
>> mStatusString[][25] = {
>>>      "Success",                      //  RETURN_SUCCESS
>> = 0
>>>      "Warning Unknown Glyph",        //
>> RETURN_WARN_UNKNOWN_GLYPH     = 1
>>>      "Warning Delete Failure",       //
>> RETURN_WARN_DELETE_FAILURE    = 2
>>>
>>
>>
>>
> 
> 
> 
> 

[PATCH 2/5] StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string

[Ard Biesheuvel]

FvIsBeingProcessed () emits a DEBUG print with the intent to print
the memory address of the FV that is being processed, but instead,
it prints the contents of an uninitialized stack variable.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 StandaloneMmPkg/Core/Dispatcher.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dispatcher.c
index 2f795d01dc1f..c99e6c04c8de 100644
--- a/StandaloneMmPkg/Core/Dispatcher.c
+++ b/StandaloneMmPkg/Core/Dispatcher.c
@@ -816,7 +816,7 @@ FvIsBeingProcessed (
 {
   KNOWN_FWVOL   *KnownFwVol;
 
-  DEBUG ((DEBUG_INFO, "FvIsBeingProcessed - 0x%08x\n", KnownFwVol));
+  DEBUG ((DEBUG_INFO, "FvIsBeingProcessed - 0x%08x\n", FwVolHeader));
 
   KnownFwVol = AllocatePool (sizeof (KNOWN_FWVOL));
   ASSERT (KnownFwVol != NULL);
-- 
2.26.2

Re: [edk2-devel] [PATCH 2/5] StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string

[Yao, Jiewen]

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Wednesday, June 10, 2020 4:18 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Subject: [edk2-devel] [PATCH 2/5] StandaloneMmPkg/Core: fix bogus FV pointer
> in DEBUG string
> 
> FvIsBeingProcessed () emits a DEBUG print with the intent to print
> the memory address of the FV that is being processed, but instead,
> it prints the contents of an uninitialized stack variable.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  StandaloneMmPkg/Core/Dispatcher.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/StandaloneMmPkg/Core/Dispatcher.c
> b/StandaloneMmPkg/Core/Dispatcher.c
> index 2f795d01dc1f..c99e6c04c8de 100644
> --- a/StandaloneMmPkg/Core/Dispatcher.c
> +++ b/StandaloneMmPkg/Core/Dispatcher.c
> @@ -816,7 +816,7 @@ FvIsBeingProcessed (
>  {
> 
>    KNOWN_FWVOL   *KnownFwVol;
> 
> 
> 
> -  DEBUG ((DEBUG_INFO, "FvIsBeingProcessed - 0x%08x\n", KnownFwVol));
> 
> +  DEBUG ((DEBUG_INFO, "FvIsBeingProcessed - 0x%08x\n", FwVolHeader));
> 
> 
> 
>    KnownFwVol = AllocatePool (sizeof (KNOWN_FWVOL));
> 
>    ASSERT (KnownFwVol != NULL);
> 
> --
> 2.26.2
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#61040): https://edk2.groups.io/g/devel/message/61040
> Mute This Topic: https://groups.io/mt/74792289/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [PATCH 2/5] StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 78 bytes --]

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

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 101 bytes --]

[PATCH 3/5] StandaloneMmPkg/Core: add missing GUID reference

[Ard Biesheuvel]

The Standalone core uses gEfiHobMemoryAllocModuleGuid, but failed to
declare this in its INF.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index 7d590b49bd3f..d17ff9965bdc 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -69,6 +69,7 @@ [Guids]
   gEdkiiMemoryProfileGuid
   gZeroGuid                                     ## SOMETIMES_CONSUMES   ## GUID
   gEfiHobListGuid
+  gEfiHobMemoryAllocModuleGuid
   gMmCoreDataHobGuid
   gMmFvDispatchGuid
   gEfiEventLegacyBootGuid
-- 
2.26.2

Re: [PATCH 3/5] StandaloneMmPkg/Core: add missing GUID reference

[Yao, Jiewen]

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Wednesday, June 10, 2020 4:18 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Subject: [PATCH 3/5] StandaloneMmPkg/Core: add missing GUID reference
> 
> The Standalone core uses gEfiHobMemoryAllocModuleGuid, but failed to
> declare this in its INF.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> index 7d590b49bd3f..d17ff9965bdc 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> @@ -69,6 +69,7 @@ [Guids]
>    gEdkiiMemoryProfileGuid
> 
>    gZeroGuid                                     ## SOMETIMES_CONSUMES   ## GUID
> 
>    gEfiHobListGuid
> 
> +  gEfiHobMemoryAllocModuleGuid
> 
>    gMmCoreDataHobGuid
> 
>    gMmFvDispatchGuid
> 
>    gEfiEventLegacyBootGuid
> 
> --
> 2.26.2

Re: [edk2-devel] [PATCH 3/5] StandaloneMmPkg/Core: add missing GUID reference

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 78 bytes --]

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

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 101 bytes --]

[PATCH 4/5] StandaloneMmPkg: generate position independent code for StMM core

[Ard Biesheuvel]

The standalone MM core runs in a restricted environment that is set
up by a higher privilege level, and which may not allow memory regions
to be writable and executable at the same time.

This means that making the StMM core self-relocatable requires that
all the targets of the relocation fixups are outside of the executable
region of the image, given that we cannot remap the executable code
writable from the executable code itself without losing those execute
permissions.

So instead, use the existing toolchain support to ensure that position
independent code is used where possible, and that all the remaining
relocated quantities are emitted into the data section. (Note that
staticallly initialized const pointers will be emitted into the
.data.rel.ro section, which gets pulled into the .data section by
our linker script)

To ensure that we don't pick up any absolute references in executable
code inadvertently (e.g., in assembler code), add the '-z text' linker
option which will force the build to fail in this case.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 StandaloneMmPkg/Core/StandaloneMmCore.inf                                         | 4 ++++
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index d17ff9965bdc..87bf6e9440a7 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -75,3 +75,7 @@ [Guids]
   gEfiEventLegacyBootGuid
   gEfiEventExitBootServicesGuid
   gEfiEventReadyToBootGuid
+
+[BuildOptions]
+  GCC:*_*_*_CC_FLAGS = -fpie
+  GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 891c292e92f8..7d6ee4e08ecb 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -48,3 +48,6 @@ [Guids]
   gEfiMmPeiMmramMemoryReserveGuid
   gEfiStandaloneMmNonSecureBufferGuid
   gEfiArmTfCpuDriverEpDescriptorGuid
+
+[BuildOptions]
+  GCC:*_*_*_CC_FLAGS = -fpie
-- 
2.26.2

Re: [edk2-devel] [PATCH 4/5] StandaloneMmPkg: generate position independent code for StMM core

[Sean]

Ard,

I see you are only doing this for GCC?

Is it not needed for VS or clang? Are these toolchains not supported for 
StandaloneMmPkg?

Not trying to hold up your work and don't expect you to enable every 
toolchain but also don't like the idea of only building out support for 
a single toolchain given all the work that has gone into making modules 
compatible with numerous toolchains.

thoughts?

Thanks
Sean



On 6/10/2020 1:17 AM, Ard Biesheuvel wrote:
> The standalone MM core runs in a restricted environment that is set
> up by a higher privilege level, and which may not allow memory regions
> to be writable and executable at the same time.
> 
> This means that making the StMM core self-relocatable requires that
> all the targets of the relocation fixups are outside of the executable
> region of the image, given that we cannot remap the executable code
> writable from the executable code itself without losing those execute
> permissions.
> 
> So instead, use the existing toolchain support to ensure that position
> independent code is used where possible, and that all the remaining
> relocated quantities are emitted into the data section. (Note that
> staticallly initialized const pointers will be emitted into the
> .data.rel.ro section, which gets pulled into the .data section by
> our linker script)
> 
> To ensure that we don't pick up any absolute references in executable
> code inadvertently (e.g., in assembler code), add the '-z text' linker
> option which will force the build to fail in this case.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>   StandaloneMmPkg/Core/StandaloneMmCore.inf                                         | 4 ++++
>   StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 3 +++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> index d17ff9965bdc..87bf6e9440a7 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> @@ -75,3 +75,7 @@ [Guids]
>     gEfiEventLegacyBootGuid
> 
>     gEfiEventExitBootServicesGuid
> 
>     gEfiEventReadyToBootGuid
> 
> +
> 
> +[BuildOptions]
> 
> +  GCC:*_*_*_CC_FLAGS = -fpie
> 
> +  GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
> 
> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> index 891c292e92f8..7d6ee4e08ecb 100644
> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> @@ -48,3 +48,6 @@ [Guids]
>     gEfiMmPeiMmramMemoryReserveGuid
> 
>     gEfiStandaloneMmNonSecureBufferGuid
> 
>     gEfiArmTfCpuDriverEpDescriptorGuid
> 
> +
> 
> +[BuildOptions]
> 
> +  GCC:*_*_*_CC_FLAGS = -fpie
> 

Re: [edk2-devel] [PATCH 4/5] StandaloneMmPkg: generate position independent code for StMM core

[Ard Biesheuvel]

On 6/10/20 8:21 PM, Sean Brogan wrote:
> Ard,
> 
> I see you are only doing this for GCC?
> 
> Is it not needed for VS or clang? Are these toolchains not supported for 
> StandaloneMmPkg?
> 

This works with Clang as well as GCC (Clang is part of the GCC toolchain 
family in EDK2)

None of the AArch64 platforms we have in EDK2 or edk2-platforms can be 
built with VS today: the problem is that the asm dialect is different, 
and we don't have .asm versions of the startup code that can be built 
with VS. I think some of the common pieces have been ported by Pete 
Batard (so you can build individual drivers for AArch64 using VS), but 
nothing else.

> Not trying to hold up your work and don't expect you to enable every 
> toolchain but also don't like the idea of only building out support for 
> a single toolchain given all the work that has gone into making modules 
> compatible with numerous toolchains.
> 
> thoughts?
> 

Leif and I tasked someone with adding this VS support when we were both 
at Linaro, but this never materialized, unfortunately. I don't have a 
Windows machine, and no bandwidth to educate myself on the use of Visual 
Studio (last version I used was 6 :-)), so I am going to leave this up 
to someone else.

That said, this alternative approach (suggested by Jiewen) was adopted 
to rely less on ELF particulars or GCC/binutils. I do think that 
building this code needs the attention of someone who understanfds the 
VS toolchain really well, though, since we need to build a standalone 
PE/COFF executable that is structured as a shared library (i.e., with no 
relocation records pointing into the executable segment), otherwise 
there is no way we can dispatch those running at S-EL0 under the Secure 
Partition Manager.

Do you have any candidates that could help me figure out the build 
options we need for VS here? Beyond that, I don't see any GCC 
dependencies in StandaloneMmPkg at all.



> 
> On 6/10/2020 1:17 AM, Ard Biesheuvel wrote:
>> The standalone MM core runs in a restricted environment that is set
>> up by a higher privilege level, and which may not allow memory regions
>> to be writable and executable at the same time.
>>
>> This means that making the StMM core self-relocatable requires that
>> all the targets of the relocation fixups are outside of the executable
>> region of the image, given that we cannot remap the executable code
>> writable from the executable code itself without losing those execute
>> permissions.
>>
>> So instead, use the existing toolchain support to ensure that position
>> independent code is used where possible, and that all the remaining
>> relocated quantities are emitted into the data section. (Note that
>> staticallly initialized const pointers will be emitted into the
>> .data.rel.ro section, which gets pulled into the .data section by
>> our linker script)
>>
>> To ensure that we don't pick up any absolute references in executable
>> code inadvertently (e.g., in assembler code), add the '-z text' linker
>> option which will force the build to fail in this case.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   
>> StandaloneMmPkg/Core/StandaloneMmCore.inf                                         
>> | 4 ++++
>>   
>> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>> | 3 +++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf 
>> b/StandaloneMmPkg/Core/StandaloneMmCore.inf
>> index d17ff9965bdc..87bf6e9440a7 100644
>> --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
>> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
>> @@ -75,3 +75,7 @@ [Guids]
>>     gEfiEventLegacyBootGuid
>>
>>     gEfiEventExitBootServicesGuid
>>
>>     gEfiEventReadyToBootGuid
>>
>> +
>>
>> +[BuildOptions]
>>
>> +  GCC:*_*_*_CC_FLAGS = -fpie
>>
>> +  GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
>>
>> diff --git 
>> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>>
>> index 891c292e92f8..7d6ee4e08ecb 100644
>> --- 
>> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>>
>> +++ 
>> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>>
>> @@ -48,3 +48,6 @@ [Guids]
>>     gEfiMmPeiMmramMemoryReserveGuid
>>
>>     gEfiStandaloneMmNonSecureBufferGuid
>>
>>     gEfiArmTfCpuDriverEpDescriptorGuid
>>
>> +
>>
>> +[BuildOptions]
>>
>> +  GCC:*_*_*_CC_FLAGS = -fpie
>>

Re: [edk2-devel] [PATCH 4/5] StandaloneMmPkg: generate position independent code for StMM core

[Yao, Jiewen]

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

I hope GCC expert can review this and double confirm.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Wednesday, June 10, 2020 4:18 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Subject: [edk2-devel] [PATCH 4/5] StandaloneMmPkg: generate position
> independent code for StMM core
> 
> The standalone MM core runs in a restricted environment that is set
> up by a higher privilege level, and which may not allow memory regions
> to be writable and executable at the same time.
> 
> This means that making the StMM core self-relocatable requires that
> all the targets of the relocation fixups are outside of the executable
> region of the image, given that we cannot remap the executable code
> writable from the executable code itself without losing those execute
> permissions.
> 
> So instead, use the existing toolchain support to ensure that position
> independent code is used where possible, and that all the remaining
> relocated quantities are emitted into the data section. (Note that
> staticallly initialized const pointers will be emitted into the
> .data.rel.ro section, which gets pulled into the .data section by
> our linker script)
> 
> To ensure that we don't pick up any absolute references in executable
> code inadvertently (e.g., in assembler code), add the '-z text' linker
> option which will force the build to fail in this case.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  StandaloneMmPkg/Core/StandaloneMmCore.inf                                         | 4 ++++
> 
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreE
> ntryPoint.inf | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> index d17ff9965bdc..87bf6e9440a7 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> @@ -75,3 +75,7 @@ [Guids]
>    gEfiEventLegacyBootGuid
> 
>    gEfiEventExitBootServicesGuid
> 
>    gEfiEventReadyToBootGuid
> 
> +
> 
> +[BuildOptions]
> 
> +  GCC:*_*_*_CC_FLAGS = -fpie
> 
> +  GCC:*_*_*_DLINK_FLAGS = -Wl,-z,text,-Bsymbolic,-pie
> 
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCor
> eEntryPoint.inf
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCor
> eEntryPoint.inf
> index 891c292e92f8..7d6ee4e08ecb 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCor
> eEntryPoint.inf
> +++
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCor
> eEntryPoint.inf
> @@ -48,3 +48,6 @@ [Guids]
>    gEfiMmPeiMmramMemoryReserveGuid
> 
>    gEfiStandaloneMmNonSecureBufferGuid
> 
>    gEfiArmTfCpuDriverEpDescriptorGuid
> 
> +
> 
> +[BuildOptions]
> 
> +  GCC:*_*_*_CC_FLAGS = -fpie
> 
> --
> 2.26.2
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#61042): https://edk2.groups.io/g/devel/message/61042
> Mute This Topic: https://groups.io/mt/74792291/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
> -=-=-=-=-=-=

[PATCH 5/5] StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the fly

[Ard Biesheuvel]

Apply PE/COFF fixups when starting up the standalone MM core, so that
it can execute at any address regardless of the link time address.

Note that this requires the PE/COFF image to be emitted with its
relocation section preserved. Special care is taken to ensure that
TE images are dealt with correctly as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h                    |  2 ++
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c             | 11 +++++++---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 22 ++++++++++++++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
index 494bcf3dc28f..a3420699e6f1 100644
--- a/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
+++ b/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
@@ -82,6 +82,7 @@ EFI_STATUS
 EFIAPI
 UpdateMmFoundationPeCoffPermissions (
   IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
+  IN  EFI_PHYSICAL_ADDRESS                    ImageBase,
   IN  UINT32                                  SectionHeaderOffset,
   IN  CONST  UINT16                           NumberOfSections,
   IN  REGION_PERMISSION_UPDATE_FUNC           TextUpdater,
@@ -107,6 +108,7 @@ EFIAPI
 GetStandaloneMmCorePeCoffSections (
   IN        VOID                            *TeData,
   IN  OUT   PE_COFF_LOADER_IMAGE_CONTEXT    *ImageContext,
+      OUT   EFI_PHYSICAL_ADDRESS            *ImageBase,
   IN  OUT   UINT32                          *SectionHeaderOffset,
   IN  OUT   UINT16                          *NumberOfSections
   );
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
index 00f49c9d0558..bf9650d54629 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c
@@ -29,6 +29,7 @@ EFI_STATUS
 EFIAPI
 UpdateMmFoundationPeCoffPermissions (
   IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
+  IN  EFI_PHYSICAL_ADDRESS                    ImageBase,
   IN  UINT32                                  SectionHeaderOffset,
   IN  CONST  UINT16                           NumberOfSections,
   IN  REGION_PERMISSION_UPDATE_FUNC           TextUpdater,
@@ -87,7 +88,7 @@ UpdateMmFoundationPeCoffPermissions (
     // if it is a writeable section then mark it appropriately as well.
     //
     if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
-      Base = ImageContext->ImageAddress + SectionHeader.VirtualAddress;
+      Base = ImageBase + SectionHeader.VirtualAddress;
 
       TextUpdater (Base, SectionHeader.Misc.VirtualSize);
 
@@ -153,6 +154,7 @@ STATIC
 EFI_STATUS
 GetPeCoffSectionInformation (
   IN  OUT   PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
+      OUT   EFI_PHYSICAL_ADDRESS              *ImageBase,
       OUT   UINT32                            *SectionHeaderOffset,
       OUT   UINT16                            *NumberOfSections
   )
@@ -212,6 +214,7 @@ GetPeCoffSectionInformation (
     return Status;
   }
 
+  *ImageBase = ImageContext->ImageAddress;
   if (!ImageContext->IsTeImage) {
     ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
 
@@ -232,7 +235,7 @@ GetPeCoffSectionInformation (
   } else {
     *SectionHeaderOffset = (UINTN)(sizeof (EFI_TE_IMAGE_HEADER));
     *NumberOfSections = Hdr.Te->NumberOfSections;
-    ImageContext->ImageAddress -= (UINT32)Hdr.Te->StrippedSize - sizeof (EFI_TE_IMAGE_HEADER);
+    *ImageBase -= (UINT32)Hdr.Te->StrippedSize - sizeof (EFI_TE_IMAGE_HEADER);
   }
   return RETURN_SUCCESS;
 }
@@ -242,6 +245,7 @@ EFIAPI
 GetStandaloneMmCorePeCoffSections (
   IN        VOID                            *TeData,
   IN  OUT   PE_COFF_LOADER_IMAGE_CONTEXT    *ImageContext,
+      OUT   EFI_PHYSICAL_ADDRESS            *ImageBase,
   IN  OUT   UINT32                          *SectionHeaderOffset,
   IN  OUT   UINT16                          *NumberOfSections
   )
@@ -255,7 +259,8 @@ GetStandaloneMmCorePeCoffSections (
 
   DEBUG ((DEBUG_INFO, "Found Standalone MM PE data - 0x%x\n", TeData));
 
-  Status = GetPeCoffSectionInformation (ImageContext, SectionHeaderOffset, NumberOfSections);
+  Status = GetPeCoffSectionInformation (ImageContext, ImageBase,
+             SectionHeaderOffset, NumberOfSections);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "Unable to locate Standalone MM Core PE-COFF Section information - %r\n", Status));
     return Status;
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 20723385113f..9cecfa667b90 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -225,6 +225,7 @@ _ModuleEntryPoint (
   VOID                                    *HobStart;
   VOID                                    *TeData;
   UINTN                                   TeDataSize;
+  EFI_PHYSICAL_ADDRESS                    ImageBase;
 
   // Get Secure Partition Manager Version Information
   Status = GetSpmVersion ();
@@ -253,6 +254,7 @@ _ModuleEntryPoint (
   Status = GetStandaloneMmCorePeCoffSections (
              TeData,
              &ImageContext,
+             &ImageBase,
              &SectionHeaderOffset,
              &NumberOfSections
              );
@@ -261,10 +263,21 @@ _ModuleEntryPoint (
     goto finish;
   }
 
+  //
+  // ImageBase may deviate from ImageContext.ImageAddress if we are dealing
+  // with a TE image, in which case the latter points to the actual offset
+  // of the image, whereas ImageBase refers to the address where the image
+  // would start if the stripped PE headers were still in place. In either
+  // case, we need to fix up ImageBase so it refers to the actual current
+  // load address.
+  //
+  ImageBase += (UINTN)TeData - ImageContext.ImageAddress;
+
   // Update the memory access permissions of individual sections in the
   // Standalone MM core module
   Status = UpdateMmFoundationPeCoffPermissions (
              &ImageContext,
+             ImageBase,
              SectionHeaderOffset,
              NumberOfSections,
              ArmSetMemoryRegionNoExec,
@@ -276,6 +289,15 @@ _ModuleEntryPoint (
     goto finish;
   }
 
+  if (ImageContext.ImageAddress != (UINTN)TeData) {
+    ImageContext.ImageAddress = (UINTN)TeData;
+    ArmSetMemoryRegionNoExec (ImageBase, SIZE_4KB);
+    ArmClearMemoryRegionReadOnly (ImageBase, SIZE_4KB);
+
+    Status = PeCoffLoaderRelocateImage (&ImageContext);
+    ASSERT_EFI_ERROR (Status);
+  }
+
   //
   // Create Hoblist based upon boot information passed by privileged software
   //
-- 
2.26.2

Re: [edk2-devel] [PATCH 5/5] StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the fly

[Yao, Jiewen]

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

I hope ARM expert can review this to double confirm.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Wednesday, June 10, 2020 4:18 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Subject: [edk2-devel] [PATCH 5/5]
> StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the
> fly
> 
> Apply PE/COFF fixups when starting up the standalone MM core, so that
> it can execute at any address regardless of the link time address.
> 
> Note that this requires the PE/COFF image to be emitted with its
> relocation section preserved. Special care is taken to ensure that
> TE images are dealt with correctly as well.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
> |  2 ++
> 
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissi
> ons.c             | 11 +++++++---
> 
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standalone
> MmCoreEntryPoint.c | 22 ++++++++++++++++++++
>  3 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git
> a/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
> b/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
> index 494bcf3dc28f..a3420699e6f1 100644
> ---
> a/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
> +++
> b/StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h
> @@ -82,6 +82,7 @@ EFI_STATUS
>  EFIAPI
> 
>  UpdateMmFoundationPeCoffPermissions (
> 
>    IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
> 
> +  IN  EFI_PHYSICAL_ADDRESS                    ImageBase,
> 
>    IN  UINT32                                  SectionHeaderOffset,
> 
>    IN  CONST  UINT16                           NumberOfSections,
> 
>    IN  REGION_PERMISSION_UPDATE_FUNC           TextUpdater,
> 
> @@ -107,6 +108,7 @@ EFIAPI
>  GetStandaloneMmCorePeCoffSections (
> 
>    IN        VOID                            *TeData,
> 
>    IN  OUT   PE_COFF_LOADER_IMAGE_CONTEXT    *ImageContext,
> 
> +      OUT   EFI_PHYSICAL_ADDRESS            *ImageBase,
> 
>    IN  OUT   UINT32                          *SectionHeaderOffset,
> 
>    IN  OUT   UINT16                          *NumberOfSections
> 
>    );
> 
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermis
> sions.c
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermis
> sions.c
> index 00f49c9d0558..bf9650d54629 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermis
> sions.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermis
> sions.c
> @@ -29,6 +29,7 @@ EFI_STATUS
>  EFIAPI
> 
>  UpdateMmFoundationPeCoffPermissions (
> 
>    IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
> 
> +  IN  EFI_PHYSICAL_ADDRESS                    ImageBase,
> 
>    IN  UINT32                                  SectionHeaderOffset,
> 
>    IN  CONST  UINT16                           NumberOfSections,
> 
>    IN  REGION_PERMISSION_UPDATE_FUNC           TextUpdater,
> 
> @@ -87,7 +88,7 @@ UpdateMmFoundationPeCoffPermissions (
>      // if it is a writeable section then mark it appropriately as well.
> 
>      //
> 
>      if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
> 
> -      Base = ImageContext->ImageAddress + SectionHeader.VirtualAddress;
> 
> +      Base = ImageBase + SectionHeader.VirtualAddress;
> 
> 
> 
>        TextUpdater (Base, SectionHeader.Misc.VirtualSize);
> 
> 
> 
> @@ -153,6 +154,7 @@ STATIC
>  EFI_STATUS
> 
>  GetPeCoffSectionInformation (
> 
>    IN  OUT   PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
> 
> +      OUT   EFI_PHYSICAL_ADDRESS              *ImageBase,
> 
>        OUT   UINT32                            *SectionHeaderOffset,
> 
>        OUT   UINT16                            *NumberOfSections
> 
>    )
> 
> @@ -212,6 +214,7 @@ GetPeCoffSectionInformation (
>      return Status;
> 
>    }
> 
> 
> 
> +  *ImageBase = ImageContext->ImageAddress;
> 
>    if (!ImageContext->IsTeImage) {
> 
>      ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
> 
> 
> 
> @@ -232,7 +235,7 @@ GetPeCoffSectionInformation (
>    } else {
> 
>      *SectionHeaderOffset = (UINTN)(sizeof (EFI_TE_IMAGE_HEADER));
> 
>      *NumberOfSections = Hdr.Te->NumberOfSections;
> 
> -    ImageContext->ImageAddress -= (UINT32)Hdr.Te->StrippedSize - sizeof
> (EFI_TE_IMAGE_HEADER);
> 
> +    *ImageBase -= (UINT32)Hdr.Te->StrippedSize - sizeof
> (EFI_TE_IMAGE_HEADER);
> 
>    }
> 
>    return RETURN_SUCCESS;
> 
>  }
> 
> @@ -242,6 +245,7 @@ EFIAPI
>  GetStandaloneMmCorePeCoffSections (
> 
>    IN        VOID                            *TeData,
> 
>    IN  OUT   PE_COFF_LOADER_IMAGE_CONTEXT    *ImageContext,
> 
> +      OUT   EFI_PHYSICAL_ADDRESS            *ImageBase,
> 
>    IN  OUT   UINT32                          *SectionHeaderOffset,
> 
>    IN  OUT   UINT16                          *NumberOfSections
> 
>    )
> 
> @@ -255,7 +259,8 @@ GetStandaloneMmCorePeCoffSections (
> 
> 
>    DEBUG ((DEBUG_INFO, "Found Standalone MM PE data - 0x%x\n", TeData));
> 
> 
> 
> -  Status = GetPeCoffSectionInformation (ImageContext, SectionHeaderOffset,
> NumberOfSections);
> 
> +  Status = GetPeCoffSectionInformation (ImageContext, ImageBase,
> 
> +             SectionHeaderOffset, NumberOfSections);
> 
>    if (EFI_ERROR (Status)) {
> 
>      DEBUG ((DEBUG_ERROR, "Unable to locate Standalone MM Core PE-COFF
> Section information - %r\n", Status));
> 
>      return Status;
> 
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standalon
> eMmCoreEntryPoint.c
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standalon
> eMmCoreEntryPoint.c
> index 20723385113f..9cecfa667b90 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standalon
> eMmCoreEntryPoint.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standalon
> eMmCoreEntryPoint.c
> @@ -225,6 +225,7 @@ _ModuleEntryPoint (
>    VOID                                    *HobStart;
> 
>    VOID                                    *TeData;
> 
>    UINTN                                   TeDataSize;
> 
> +  EFI_PHYSICAL_ADDRESS                    ImageBase;
> 
> 
> 
>    // Get Secure Partition Manager Version Information
> 
>    Status = GetSpmVersion ();
> 
> @@ -253,6 +254,7 @@ _ModuleEntryPoint (
>    Status = GetStandaloneMmCorePeCoffSections (
> 
>               TeData,
> 
>               &ImageContext,
> 
> +             &ImageBase,
> 
>               &SectionHeaderOffset,
> 
>               &NumberOfSections
> 
>               );
> 
> @@ -261,10 +263,21 @@ _ModuleEntryPoint (
>      goto finish;
> 
>    }
> 
> 
> 
> +  //
> 
> +  // ImageBase may deviate from ImageContext.ImageAddress if we are
> dealing
> 
> +  // with a TE image, in which case the latter points to the actual offset
> 
> +  // of the image, whereas ImageBase refers to the address where the image
> 
> +  // would start if the stripped PE headers were still in place. In either
> 
> +  // case, we need to fix up ImageBase so it refers to the actual current
> 
> +  // load address.
> 
> +  //
> 
> +  ImageBase += (UINTN)TeData - ImageContext.ImageAddress;
> 
> +
> 
>    // Update the memory access permissions of individual sections in the
> 
>    // Standalone MM core module
> 
>    Status = UpdateMmFoundationPeCoffPermissions (
> 
>               &ImageContext,
> 
> +             ImageBase,
> 
>               SectionHeaderOffset,
> 
>               NumberOfSections,
> 
>               ArmSetMemoryRegionNoExec,
> 
> @@ -276,6 +289,15 @@ _ModuleEntryPoint (
>      goto finish;
> 
>    }
> 
> 
> 
> +  if (ImageContext.ImageAddress != (UINTN)TeData) {
> 
> +    ImageContext.ImageAddress = (UINTN)TeData;
> 
> +    ArmSetMemoryRegionNoExec (ImageBase, SIZE_4KB);
> 
> +    ArmClearMemoryRegionReadOnly (ImageBase, SIZE_4KB);
> 
> +
> 
> +    Status = PeCoffLoaderRelocateImage (&ImageContext);
> 
> +    ASSERT_EFI_ERROR (Status);
> 
> +  }
> 
> +
> 
>    //
> 
>    // Create Hoblist based upon boot information passed by privileged software
> 
>    //
> 
> --
> 2.26.2
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#61043): https://edk2.groups.io/g/devel/message/61043
> Mute This Topic: https://groups.io/mt/74792292/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [PATCH 5/5] StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the fly

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

Apologies for top replying. I am not sure if it is my setup where I see additional characters (e.g. =0D at the end of each line) which appear to confuse my email client.

> UpdateMmFoundationPeCoffPermissions (=0D
> IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,=0D
> + IN EFI_PHYSICAL_ADDRESS ImageBase,=0D
Can the function documentation for UpdateMmFoundationPeCoffPermissions() and GetStandaloneMmCorePeCoffSections() be updated to reflect the additional parameter ImageBase, please?

On Wed, Jun 10, 2020 at 01:17 AM, Ard Biesheuvel wrote:

> 
> + *ImageBase =3D ImageContext->ImageAddress;=0D

I think the '*ImageBase = ImageContext->ImageAddress;' statement can be moved inside the if condition.
With these changes.

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

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 37150 bytes --]

Re: [edk2-devel] [PATCH 5/5] StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the fly

[Ard Biesheuvel]

On 6/15/20 3:59 PM, Sami Mujawar via Groups.Io wrote:
> Apologies for top replying. I am not sure if it is my setup where I see 
> additional characters (e.g. =0D at the end of each line) which appear to 
> confuse my email client.
> 

That must be Thunderbird playing tricks.

> 
>  > UpdateMmFoundationPeCoffPermissions (=0D
>  > IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,=0D
>  > + IN EFI_PHYSICAL_ADDRESS ImageBase,=0D
> Can the function documentation for UpdateMmFoundationPeCoffPermissions() 
> and GetStandaloneMmCorePeCoffSections() be updated to reflect the 
> additional parameter ImageBase, please?
> 

Sure

> On Wed, Jun 10, 2020 at 01:17 AM, Ard Biesheuvel wrote:
> 
>     + *ImageBase =3D ImageContext->ImageAddress;=0D
> 
> I think the '*ImageBase = ImageContext->ImageAddress;' statement can be 
> moved inside the if condition.

No, the TE branch of the if() does a subtraction so it needs the value 
to be set beforehand.




> With these changes.
> 
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> 
> Sami Mujawar

Re: [edk2-devel] [PATCH 5/5] StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the fly

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Mon, Jun 15, 2020 at 07:12 AM, Ard Biesheuvel wrote:

> 
> 
>> On Wed, Jun 10, 2020 at 01:17 AM, Ard Biesheuvel wrote:
>> + *ImageBase =3D ImageContext->ImageAddress;=0D
>> I think the '*ImageBase = ImageContext->ImageAddress;' statement can be
>> moved inside the if condition.
> 
> No, the TE branch of the if() does a subtraction so it needs the value to
> be set beforehand.

Sorry, I missed the -=. Please ignore.

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 523 bytes --]

Re: [PATCH 0/5] StandaloneMmPkg: make StMM core relocatable

[Ilias Apalodimas]

Hi Ard, 

Tested on QEMU with the op-tee patches i mentioned in my RFC [1]
Everything seems to work correctly


[1] https://edk2.groups.io/g/devel/message/60835

On Wed, Jun 10, 2020 at 10:17:35AM +0200, Ard Biesheuvel wrote:
> It is not always possible to deploy the standalone MM core in a way where
> the runtime address is known at build time. This does not matter for most
> modules, since they are relocated at dispatch time. However, for the MM
> core itself, it means we need to do some extra work to relocate the image
> in place if it ends up at a different offset than expected.
> 
> On AARCH64, the standalone MM stack is deployed inside a non-privileged
> secure world container which only has limited control over its memory
> mappings, and so we need to ensure that the executable code itself is
> free of absolute quantities that need to be fixed up. This is very similar
> to how shared libraries are constructed, given that pages can only be
> shared between processes if they are not modified, even by the dynamic
> loader. So we can use this support to emit the standaline MM core in a
> way that guarantees that the executable code does not need to modify
> itself (patch #4)
> 
> Patch #5 adds the actual code to perform the self relocation after the
> .data section has been made writable and non-executable. Note that the
> PE/COFF library code modifies the header in place, and so in the case
> where we need to perform the runtime relocation, we need to remap the
> header page writable and non-executable as well.
> 
> The remaining patches are optimizations and fixes I picked up along
> the way.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Ard Biesheuvel (5):
>   MdePkg/BasePrintLib: avoid absolute addresses for error strings
>   StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string
>   StandaloneMmPkg/Core: add missing GUID reference
>   StandaloneMmPkg: generate position independent code for StMM core
>   StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the
>     fly
> 
>  StandaloneMmPkg/Core/StandaloneMmCore.inf                                               |  5 +++++
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |  3 +++
>  StandaloneMmPkg/Include/Library/AArch64/StandaloneMmCoreEntryPoint.h                    |  2 ++
>  MdePkg/Library/BasePrintLib/PrintLibInternal.c                                          |  2 +-
>  StandaloneMmPkg/Core/Dispatcher.c                                                       |  2 +-
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c             | 11 +++++++---
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 22 ++++++++++++++++++++
>  7 files changed, 42 insertions(+), 5 deletions(-)
> 
> -- 
> 2.26.2
> 

Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 0/5] StandaloneMmPkg: make StMM core relocatable

[Ard Biesheuvel]

On 6/10/20 10:17 AM, Ard Biesheuvel wrote:
> It is not always possible to deploy the standalone MM core in a way where
> the runtime address is known at build time. This does not matter for most
> modules, since they are relocated at dispatch time. However, for the MM
> core itself, it means we need to do some extra work to relocate the image
> in place if it ends up at a different offset than expected.
> 
> On AARCH64, the standalone MM stack is deployed inside a non-privileged
> secure world container which only has limited control over its memory
> mappings, and so we need to ensure that the executable code itself is
> free of absolute quantities that need to be fixed up. This is very similar
> to how shared libraries are constructed, given that pages can only be
> shared between processes if they are not modified, even by the dynamic
> loader. So we can use this support to emit the standaline MM core in a
> way that guarantees that the executable code does not need to modify
> itself (patch #4)
> 
> Patch #5 adds the actual code to perform the self relocation after the
> .data section has been made writable and non-executable. Note that the
> PE/COFF library code modifies the header in place, and so in the case
> where we need to perform the runtime relocation, we need to remap the
> header page writable and non-executable as well.
> 
> The remaining patches are optimizations and fixes I picked up along
> the way.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 

Any thoughts from the StandaloneMmPkg co-maintainers?

Re: [PATCH 0/5] StandaloneMmPkg: make StMM core relocatable

[Ard Biesheuvel]

On 6/10/20 10:17 AM, Ard Biesheuvel wrote:
> It is not always possible to deploy the standalone MM core in a way where
> the runtime address is known at build time. This does not matter for most
> modules, since they are relocated at dispatch time. However, for the MM
> core itself, it means we need to do some extra work to relocate the image
> in place if it ends up at a different offset than expected.
> 
> On AARCH64, the standalone MM stack is deployed inside a non-privileged
> secure world container which only has limited control over its memory
> mappings, and so we need to ensure that the executable code itself is
> free of absolute quantities that need to be fixed up. This is very similar
> to how shared libraries are constructed, given that pages can only be
> shared between processes if they are not modified, even by the dynamic
> loader. So we can use this support to emit the standaline MM core in a
> way that guarantees that the executable code does not need to modify
> itself (patch #4)
> 
> Patch #5 adds the actual code to perform the self relocation after the
> .data section has been made writable and non-executable. Note that the
> PE/COFF library code modifies the header in place, and so in the case
> where we need to perform the runtime relocation, we need to remap the
> header page writable and non-executable as well.
> 
> The remaining patches are optimizations and fixes I picked up along
> the way.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Ard Biesheuvel (5):
>    MdePkg/BasePrintLib: avoid absolute addresses for error strings
>    StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string
>    StandaloneMmPkg/Core: add missing GUID reference
>    StandaloneMmPkg: generate position independent code for StMM core
>    StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the
>      fly
> 

Patches 2-5 merged as #702 (patch #1 was respun separately, and merged 
as #699 earlier)

Thanks all