[edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

Serial port used by the DEBUG macro is initialised in StandaloneMmMain
by the DebugLib constructor.

When we use a serial port initialised by TF-A it is not a problem.
However, if we use a serial port that is not initialised by TF-A,
the debug log prints hangs.

Therefore, initialise the serial port early on in the entry point.

Signed-off-by: levi.yun <yeoreum.yun@arm.com>
---
These changes can be seen at
https://github.com/LeviYeoReum/edk2/tree/levi/2956_init_serial too.

 StandaloneMmPkg/StandaloneMmPkg.dsc                                                 | 1 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf   | 1 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 8012f93b7dcc38ea8fdd2de98912bbc09157ec53..040a4aa5b3d268fdfaaec9a975cfc6ff31aa37b4 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -66,6 +66,7 @@ [LibraryClasses.AARCH64, LibraryClasses.ARM]
   ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
   CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf
   PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
+  SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf

   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
   NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 75cfb98c0e75cd7cee2a59723035679612da4528..086639ecfbc983627aed73817815e2485104375e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -42,6 +42,7 @@ [LibraryClasses]
   DebugLib

 [LibraryClasses.ARM, LibraryClasses.AARCH64]
+  SerialPortLib
   StandaloneMmMmuLib
   ArmSvcLib

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index 96de10405af829c66e3f43ed4692f785d8df113e..66b56bdfe4959d5ab6152ff024caa6e900e7a948 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -335,6 +335,9 @@ _ModuleEntryPoint (
   UINTN                           TeDataSize;
   EFI_PHYSICAL_ADDRESS            ImageBase;

+  // Initialize the Serial Port early to print debug log before StandaloneMmMain.
+  SerialPortInitialize ();
+
   // Get Secure Partition Manager Version Information
   Status = GetSpmVersion ();
   if (EFI_ERROR (Status)) {
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113276): https://edk2.groups.io/g/devel/message/113276
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

+ ardb+tianocore@kernel.org

________________________________________
From: levi.yun <yeoreum.yun@arm.com>
Sent: 05 January 2024 11:49
To: devel@edk2.groups.io
Cc: Yeo Reum Yun; ard.biesheuvel@linaro.org; Sami Mujawar; ray.ni@intel.com; Pierre Gondois; nd
Subject: [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

Serial port used by the DEBUG macro is initialised in StandaloneMmMain
by the DebugLib constructor.

When we use a serial port initialised by TF-A it is not a problem.
However, if we use a serial port that is not initialised by TF-A,
the debug log prints hangs.

Therefore, initialise the serial port early on in the entry point.

Signed-off-by: levi.yun <yeoreum.yun@arm.com>
---
These changes can be seen at
https://github.com/LeviYeoReum/edk2/tree/levi/2956_init_serial too.

 StandaloneMmPkg/StandaloneMmPkg.dsc                                                 | 1 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf   | 1 +
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 8012f93b7dcc38ea8fdd2de98912bbc09157ec53..040a4aa5b3d268fdfaaec9a975cfc6ff31aa37b4 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -66,6 +66,7 @@ [LibraryClasses.AARCH64, LibraryClasses.ARM]
   ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
   CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf
   PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
+  SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf

   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
   NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 75cfb98c0e75cd7cee2a59723035679612da4528..086639ecfbc983627aed73817815e2485104375e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -42,6 +42,7 @@ [LibraryClasses]
   DebugLib

 [LibraryClasses.ARM, LibraryClasses.AARCH64]
+  SerialPortLib
   StandaloneMmMmuLib
   ArmSvcLib

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index 96de10405af829c66e3f43ed4692f785d8df113e..66b56bdfe4959d5ab6152ff024caa6e900e7a948 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -335,6 +335,9 @@ _ModuleEntryPoint (
   UINTN                           TeDataSize;
   EFI_PHYSICAL_ADDRESS            ImageBase;

+  // Initialize the Serial Port early to print debug log before StandaloneMmMain.
+  SerialPortInitialize ();
+
   // Get Secure Partition Manager Version Information
   Status = GetSpmVersion ();
   if (EFI_ERROR (Status)) {
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113277): https://edk2.groups.io/g/devel/message/113277
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[Ard Biesheuvel]

On Fri, 5 Jan 2024 at 12:49, levi.yun <yeoreum.yun@arm.com> wrote:
>
> Serial port used by the DEBUG macro is initialised in StandaloneMmMain
> by the DebugLib constructor.
>
> When we use a serial port initialised by TF-A it is not a problem.
> However, if we use a serial port that is not initialised by TF-A,
> the debug log prints hangs.
>
> Therefore, initialise the serial port early on in the entry point.
>
> Signed-off-by: levi.yun <yeoreum.yun@arm.com>
> ---
> These changes can be seen at
> https://github.com/LeviYeoReum/edk2/tree/levi/2956_init_serial too.
>

So now we will always initialize the serial port in the entrypoint
only because DebugLib might use it later with doing the
initialization.

That doesn't sound quite correct to me.

Could you explain why we cannot rely on DebugLib to call the
initializer / constructor at the right time?



>  StandaloneMmPkg/StandaloneMmPkg.dsc                                                 | 1 +
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf   | 1 +
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 3 +++
>  3 files changed, 5 insertions(+)
>
> diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
> index 8012f93b7dcc38ea8fdd2de98912bbc09157ec53..040a4aa5b3d268fdfaaec9a975cfc6ff31aa37b4 100644
> --- a/StandaloneMmPkg/StandaloneMmPkg.dsc
> +++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
> @@ -66,6 +66,7 @@ [LibraryClasses.AARCH64, LibraryClasses.ARM]
>    ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
>    CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf
>    PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
> +  SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
>
>    NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>    NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> index 75cfb98c0e75cd7cee2a59723035679612da4528..086639ecfbc983627aed73817815e2485104375e 100644
> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> @@ -42,6 +42,7 @@ [LibraryClasses]
>    DebugLib
>
>  [LibraryClasses.ARM, LibraryClasses.AARCH64]
> +  SerialPortLib
>    StandaloneMmMmuLib
>    ArmSvcLib
>
> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
> index 96de10405af829c66e3f43ed4692f785d8df113e..66b56bdfe4959d5ab6152ff024caa6e900e7a948 100644
> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
> @@ -335,6 +335,9 @@ _ModuleEntryPoint (
>    UINTN                           TeDataSize;
>    EFI_PHYSICAL_ADDRESS            ImageBase;
>
> +  // Initialize the Serial Port early to print debug log before StandaloneMmMain.
> +  SerialPortInitialize ();
> +
>    // Get Secure Partition Manager Version Information
>    Status = GetSpmVersion ();
>    if (EFI_ERROR (Status)) {
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113299): https://edk2.groups.io/g/devel/message/113299
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

Hi Ard :)

> So now we will always initialize the serial port in the entrypoint
> only because DebugLib might use it later with doing the
> initialization.
>
> That doesn't sound quite correct to me.
>
> Could you explain why we cannot rely on DebugLib to call the
> initializer / constructor at the right time?
Because, DebugLib constructor which use serial port is called in
StandAloneMmMain function.
But, this constrcutor is in _ModuleEntryPoint in StandAloneMmCoreEntry.

That means all DEBUG used in _ModuleEntryPoint can use uninitialized
serial port.
one of typical example is GetSpmVersion function.

     _ModuleEntryPoint (in StandAloneMmCoreEntry)

      // Hazard Area start
         GetSpmVersion
             DEBUG (DEBUG_INFO, xxx)  // It could be use uninitalized
Serial port.

         ...
     //  Hazard Area end
         ProcessModuleEntryPointList (StandAloneMmMain)
             ProcessLibraryConstructorList // Here. call DebugLib
constructor with SerialPortIntialize

When you see above, I would be clear. between Hazard Area Start to
Hazard Area End.
DEBUG macro would use uninitailized Serial port if that's not
initialized by TF-A.

So, It should be call SerialPortInitialized at the _ModuleEntryPoint.








IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113313): https://edk2.groups.io/g/devel/message/113313
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[Oliver Smith-Denny]

On 1/5/2024 9:22 AM, levi.yun wrote:
> 
> Hi Ard :)
> 
>> So now we will always initialize the serial port in the entrypoint
>> only because DebugLib might use it later with doing the
>> initialization.
>>
>> That doesn't sound quite correct to me.
>>
>> Could you explain why we cannot rely on DebugLib to call the
>> initializer / constructor at the right time?
> Because, DebugLib constructor which use serial port is called in
> StandAloneMmMain function.
> But, this constrcutor is in _ModuleEntryPoint in StandAloneMmCoreEntry.
> 
> That means all DEBUG used in _ModuleEntryPoint can use uninitialized
> serial port.
> one of typical example is GetSpmVersion function.
> 
>      _ModuleEntryPoint (in StandAloneMmCoreEntry)
> 
>       // Hazard Area start
>          GetSpmVersion
>              DEBUG (DEBUG_INFO, xxx)  // It could be use uninitalized
> Serial port.
> 
>          ...
>      //  Hazard Area end
>          ProcessModuleEntryPointList (StandAloneMmMain)
>              ProcessLibraryConstructorList // Here. call DebugLib
> constructor with SerialPortIntialize
> 
> When you see above, I would be clear. between Hazard Area Start to
> Hazard Area End.
> DEBUG macro would use uninitailized Serial port if that's not
> initialized by TF-A.
> 
> So, It should be call SerialPortInitialized at the _ModuleEntryPoint.

+ Laszlo

This sounds very similar to our DxeCore early serial logging discussion
a couple months ago :).

Laszlo wrote up a good summary here: 
https://edk2.groups.io/g/devel/topic/101203427#109235.

If I am understanding correctly, this would be the "lower left" in
Laszlo's diagram.

Standalone MM is likely smaller missing window than in DxeCore, but
some important information could be lost there (like the SPM version
called out, which could be very important for debugging early crashes).

So this goes back to should be we have a more generic solution for the
cores to use early logging, by fixing the SerialPortLibs? I'll parse
this more and reread the old thread further, still paging the info back
in.

Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113314): https://edk2.groups.io/g/devel/message/113314
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[Laszlo Ersek]

On 1/5/24 19:38, Oliver Smith-Denny wrote:
> On 1/5/2024 9:22 AM, levi.yun wrote:
>>
>> Hi Ard :)
>>
>>> So now we will always initialize the serial port in the entrypoint
>>> only because DebugLib might use it later with doing the
>>> initialization.
>>>
>>> That doesn't sound quite correct to me.
>>>
>>> Could you explain why we cannot rely on DebugLib to call the
>>> initializer / constructor at the right time?
>> Because, DebugLib constructor which use serial port is called in
>> StandAloneMmMain function.
>> But, this constrcutor is in _ModuleEntryPoint in StandAloneMmCoreEntry.
>>
>> That means all DEBUG used in _ModuleEntryPoint can use uninitialized
>> serial port.
>> one of typical example is GetSpmVersion function.
>>
>>      _ModuleEntryPoint (in StandAloneMmCoreEntry)
>>
>>       // Hazard Area start
>>          GetSpmVersion
>>              DEBUG (DEBUG_INFO, xxx)  // It could be use uninitalized
>> Serial port.
>>
>>          ...
>>      //  Hazard Area end
>>          ProcessModuleEntryPointList (StandAloneMmMain)
>>              ProcessLibraryConstructorList // Here. call DebugLib
>> constructor with SerialPortIntialize
>>
>> When you see above, I would be clear. between Hazard Area Start to
>> Hazard Area End.
>> DEBUG macro would use uninitailized Serial port if that's not
>> initialized by TF-A.
>>
>> So, It should be call SerialPortInitialized at the _ModuleEntryPoint.
>
> + Laszlo
>
> This sounds very similar to our DxeCore early serial logging discussion
> a couple months ago :).
>
> Laszlo wrote up a good summary here:
> https://edk2.groups.io/g/devel/topic/101203427#109235.
>
> If I am understanding correctly, this would be the "lower left" in
> Laszlo's diagram.
>
> Standalone MM is likely smaller missing window than in DxeCore, but
> some important information could be lost there (like the SPM version
> called out, which could be very important for debugging early crashes).
>
> So this goes back to should be we have a more generic solution for the
> cores to use early logging, by fixing the SerialPortLibs? I'll parse
> this more and reread the old thread further, still paging the info back
> in.

My personal conclusion in that thread was [1], and correspondingly,
commit 5087a0773645 ("ArmVirtPkg/FdtPL011SerialPortLib: initialize
implicitly", 2023-10-07). In the end, the only tractable solution was to
initialize the serial port (hardware, and library instance) exactly
once, in (a) the constructor, or (b) the explicit SerialPortInitialize()
call, or (c) any SerialPortLib API, whichever occurred first. (And (a)
and (b) can be coalesced, because SerialPortInitialize() can be marked
as the constructor for the lib instance.)

[1] http://mid.mail-archive.com/542db9e1-cd28-27a2-3a98-5b0c85cd7c79@redhat.com
    https://edk2.groups.io/g/devel/message/109235

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113386): https://edk2.groups.io/g/devel/message/113386
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

> My personal conclusion in that thread was [1], and correspondingly,
> commit 5087a0773645 ("ArmVirtPkg/FdtPL011SerialPortLib: initialize
> implicitly", 2023-10-07). In the end, the only tractable solution was to
> initialize the serial port (hardware, and library instance) exactly
> once, in (a) the constructor, or (b) the explicit SerialPortInitialize()
> call, or (c) any SerialPortLib API, whichever occurred first. (And (a)
> and (b) can be coalesced, because SerialPortInitialize() can be marked
> as the constructor for the lib instance.)
>
> [1] http://mid.mail-archive.com/542db9e1-cd28-27a2-3a98-5b0c85cd7c79@redhat.com
>      https://edk2.groups.io/g/devel/message/109235
>
> Laszlo
>
In my personal thinking, It's better to make new interface like

RETURN_STATUS
EFIAPI
SerialPortInitializeEarly (
VOID
   );

to solve this problem.

Because, It makes a memory permission fault
when we call SerialPortInitialize like
ArmVirtPkg/Library/FdtPL011SerialPortLib
where try to modify global variable.

At the _ModuleEntryPoint of StandAloneMmCore which is FIRST entry from TF-A
All of Image area is mapped as RO+X, so before load StandaloneMmCore,
We couldn't write global variable.

the purposes of above interface are:
     - Initalize serial port to use in early environment only (like
StandAloneMmCore Entry Point) where couldn't write global variable by
static information (FixedPcd).
     - It presume that all setting configured by it will be overwritten
by SerialPortInitialize.




IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113534): https://edk2.groups.io/g/devel/message/113534
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[Laszlo Ersek]

On 1/10/24 17:13, levi.yun wrote:
>> My personal conclusion in that thread was [1], and correspondingly,
>> commit 5087a0773645 ("ArmVirtPkg/FdtPL011SerialPortLib: initialize
>> implicitly", 2023-10-07). In the end, the only tractable solution was to
>> initialize the serial port (hardware, and library instance) exactly
>> once, in (a) the constructor, or (b) the explicit SerialPortInitialize()
>> call, or (c) any SerialPortLib API, whichever occurred first. (And (a)
>> and (b) can be coalesced, because SerialPortInitialize() can be marked
>> as the constructor for the lib instance.)
>>
>> [1]
>> http://mid.mail-archive.com/542db9e1-cd28-27a2-3a98-5b0c85cd7c79@redhat.com
>>      https://edk2.groups.io/g/devel/message/109235
>>
>> Laszlo
>>
> In my personal thinking, It's better to make new interface like
> 
> RETURN_STATUS
> EFIAPI
> SerialPortInitializeEarly (
> VOID
>   );
> 
> to solve this problem.
> 
> Because, It makes a memory permission fault
> when we call SerialPortInitialize like
> ArmVirtPkg/Library/FdtPL011SerialPortLib
> where try to modify global variable.
> 
> At the _ModuleEntryPoint of StandAloneMmCore which is FIRST entry from TF-A
> All of Image area is mapped as RO+X, so before load StandaloneMmCore,
> We couldn't write global variable.
> 
> the purposes of above interface are:
>     - Initalize serial port to use in early environment only (like
> StandAloneMmCore Entry Point) where couldn't write global variable by
> static information (FixedPcd).
>     - It presume that all setting configured by it will be overwritten
> by SerialPortInitialize.

This is not a scalable solution (assuming I understand your proposal
right). It would require the SerialPortLib *class* (i.e., header) to
grow a new API called SerialPortInitializeEarly(), and then all existent
SerialPortLib *instances* -- and there are too many of them -- would
have to implement that function, even if the new function were empty in
several particular library instances.

If you don't have writeable global variables at the time
SerialPortInitialize() is called, then there are two options:

(a) the common option is to just not use global variables for caching
state, and to perform the serial port init on every API call.

(b) A somewhat nasty / limited, but functional approach could be to
check the page protection covering the global variable. Something like this:

STATIC BOOLEAN  mInitialized;

...

  UINTN    AddressOfGlobal;
  BOOLEAN  GlobalsWriteable;

  if (mInitialized) {
    //
    // we're done, nothing to be done
    //
    return RETURN_SUCCESS;
  }

  //
  // here, perform the serial port initialization
  //
  ...

  //
  // finally:
  //
  AddressOfGlobal = (UINTN)&mInitialized;
  GlobalsWriteable = CheckWritable (AddressOfGlobal);
  if (GlobalsWriteable) {
    mInitialized = TRUE;
  }

This will keep initing the serial port upon every API call until the
global variable becomes writeable, and then the next API call will init
the serial port for one last time, and also prevent further page table
checks.

The CheckWritable() function is an implementation detail. In the DXE
phase, it could be implemented (I think?) with the
GetMemorySpaceDescriptor() DXE service, or perhaps even the
EFI_MEMORY_ATTRIBUTE_PROTOCOL.GetMemoryAttributes() UEFI protocol member
function. In the standalone MM core, CheckWritable() could walk the page
tables explicitly. The idea is, either way, to *predict* whether writing
to "mInitialized" would trap.

Now I think that speculative / out of order execution could actually
trigger the trap *before* GlobalsWriteable is calculated; however, I
think such a trap should be architecturally hidden (i.e., invisible). I
think at worst we could need a compiler barrier (maybe throw in some
"volatile" for GlobalsWriteable and mInitialized), so that the
*compiler* not try to reorder the accesses. But even that sounds like a
stretch.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113535): https://edk2.groups.io/g/devel/message/113535
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

Hi, Laszlo.
> This will keep initing the serial port upon every API call until the
> global variable becomes writeable, and then the next API call will init
> the serial port for one last time, and also prevent further page table
> checks.
>
> The CheckWritable() function is an implementation detail. In the DXE
> phase, it could be implemented (I think?) with the
> GetMemorySpaceDescriptor() DXE service, or perhaps even the
> EFI_MEMORY_ATTRIBUTE_PROTOCOL.GetMemoryAttributes() UEFI protocol member
> function. In the standalone MM core, CheckWritable() could walk the page
> tables explicitly. The idea is, either way, to *predict* whether writing
> to "mInitialized" would trap.
I think it wouldn't proper, to DxeCore and StMM too.
IIUC,  before CoreInitializeMemoryServices is called, we couldn't use
that method in case DxeCore.
And the problem now I face is also StMM before populating memory
information (done in LibConstructor).


> Now I think that speculative / out of order execution could actually
> trigger the trap *before* GlobalsWriteable is calculated; however, I
> think such a trap should be architecturally hidden (i.e., invisible). I
> think at worst we could need a compiler barrier (maybe throw in some
> "volatile" for GlobalsWriteable and mInitialized), so that the
> *compiler* not try to reorder the accesses. But even that sounds like a
> stretch.
Agree if we develop CheckPerm??

Currently, (In my narrow thinking) there is no more generic solution
than create new interface SerialPortInitilizeEarly.

Am I missing?

Many Thanks.

--------
Sincerely,
Levi.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113537): https://edk2.groups.io/g/devel/message/113537
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[Brian J. Johnson]

On 1/10/24 11:16, levi.yun wrote:
> Hi, Laszlo.
>> This will keep initing the serial port upon every API call until the
>> global variable becomes writeable, and then the next API call will init
>> the serial port for one last time, and also prevent further page table
>> checks.
>>
>> The CheckWritable() function is an implementation detail. In the DXE
>> phase, it could be implemented (I think?) with the
>> GetMemorySpaceDescriptor() DXE service, or perhaps even the
>> EFI_MEMORY_ATTRIBUTE_PROTOCOL.GetMemoryAttributes() UEFI protocol member
>> function. In the standalone MM core, CheckWritable() could walk the page
>> tables explicitly. The idea is, either way, to *predict* whether writing
>> to "mInitialized" would trap. >> I think it wouldn't proper, to DxeCore and StMM too.
> IIUC,  before CoreInitializeMemoryServices is called, we couldn't use
> that method in case DxeCore.
> And the problem now I face is also StMM before populating memory
> information (done in LibConstructor).
> 
> 
>> Now I think that speculative / out of order execution could actually
>> trigger the trap *before* GlobalsWriteable is calculated; however, I
>> think such a trap should be architecturally hidden (i.e., invisible). I
>> think at worst we could need a compiler barrier (maybe throw in some
>> "volatile" for GlobalsWriteable and mInitialized), so that the
>> *compiler* not try to reorder the accesses. But even that sounds like a
>> stretch.
> Agree if we develop CheckPerm??
> 
> Currently, (In my narrow thinking) there is no more generic solution
> than create new interface SerialPortInitilizeEarly.
> 
> Am I missing?
> 
> Many Thanks.
> 
> --------
> Sincerely,
> Levi.

Levi,

FWIW I prefer your original approach:  explicitly call 
SerialPortInitialize() early enough that you don't need to worry about 
output occurring before that point.  Then you can also use a 
SerialPortLib instance which assumes that this has already been done and 
doesn't try to re-initialize the port, which saves some overhead. 
Constructors in DebugLib, SerialPortLib, and other very low-level 
libraries are problematic due to the issue you ran in to, so it seems 
best to just avoid them altogether.

Ard didn't want a SerialPortInitialize() call directly in the 
all-platform StandaloneMmCore _ModuleEntryPoint() function, which is 
understandable.  So perhaps you could either:

1. Propose a platform-specific callout at that point and a library class 
to implement it, with an empty instance for general use and your own 
platform-specific instance which calls SerialPortInitialize().

or

2. Write your own platform-specific version of StandaloneMmCoreEntryPoint.

-- 
Brian J. Johnson
Enterprise X86 Lab
Hewlett Packard Enterprise



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113540): https://edk2.groups.io/g/devel/message/113540
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

Hi Brian.


> Ard didn't want a SerialPortInitialize() call directly in the
> all-platform StandaloneMmCore _ModuleEntryPoint() function, which is
> understandable.  So perhaps you could either:

Thanks to corret me :)

> 1. Propose a platform-specific callout at that point and a library class
> to implement it, with an empty instance for general use and your own
> platform-specific instance which calls SerialPortInitialize().

Thanks for suggestion.
IIUC, It looks good to me to make something like
PlatformEalryInitialize hook called in early stage of StandaloneMm
and let it be implemented on each edk2-platform to initialize Serialport early and etc.

Many thanks!





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113604): https://edk2.groups.io/g/devel/message/113604
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

Hi, Ard

Could I process with this way?

Many thanks!


On 11/01/2024 09:05, levi.yun via groups.io wrote:
> Hi Brian.
>
>
>> Ard didn't want a SerialPortInitialize() call directly in the
>> all-platform StandaloneMmCore _ModuleEntryPoint() function, which is
>> understandable.  So perhaps you could either:
> Thanks to corret me :)
>
>> 1. Propose a platform-specific callout at that point and a library class
>> to implement it, with an empty instance for general use and your own
>> platform-specific instance which calls SerialPortInitialize().
> Thanks for suggestion.
> IIUC, It looks good to me to make something like
> PlatformEalryInitialize hook called in early stage of StandaloneMm
> and let it be implemented on each edk2-platform to initialize Serialport early and etc.
>
> Many thanks!
>
>
>
>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
>
> 
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113991): https://edk2.groups.io/g/devel/message/113991
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-