Re: [PATCH 2/3] ArmPlatformPkg: PrePeiCore: write early hello message to the serial port

["Oliver Steffen" <osteffen@redhat.com>]

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

Hi everyone,

I checked Rebecca's patch with my Qemu use case.
All fine - we can use that patch instead of the series I posted here.
Even better, because it prints the version information.

Thanks!

Regards,
 Oliver

On Tue, Jul 26, 2022 at 6:22 PM Sami Mujawar <Sami.Mujawar@arm.com> wrote:

> Hi Oliver,
>
>
>
> Both your and Rebecca’s patches are using SerialPortWrite() which should
> work regardless of the debug print level and for release builds as well.
>
> Rebecca’s patch additionally initialises the serial port which is required
> if the UART has not been setup by any previous firmware.
>
> I believe Qemu might handoff with the UART initialised, therefore you
> might not see the difference. However, this would be required for some
> hardware platforms.
>
>
>
> I do not have a setup with Qemu ready to try. Is it possible for you to
> apply Rebecca’s patch and check, please?
>
> Alternatively, I will prepare a setup tomorrow to confirm.
>
>
>
> Regards,
>
>
>
> Sami Mujawar
>
>
>
> *From: *Oliver Steffen <osteffen@redhat.com>
> *Date: *Tuesday, 26 July 2022 at 16:50
> *To: *Sami Mujawar <Sami.Mujawar@arm.com>
> *Cc: *"devel@edk2.groups.io" <devel@edk2.groups.io>, Rebecca Cran <
> rebecca@bsdio.com>, "quic_rcran@quicinc.com" <quic_rcran@quicinc.com>,
> Ard Biesheuvel <ardb+tianocore@kernel.org>, Chasel Chiu <
> chasel.chiu@intel.com>, Gerd Hoffmann <kraxel@redhat.com>, Leif Lindholm <
> quic_llindhol@quicinc.com>, Nate DeSimone <nathaniel.l.desimone@intel.com>,
> Star Zeng <star.zeng@intel.com>, Andrew Fish <afish@apple.com>, Laszlo
> Ersek <lersek@redhat.com>
> *Subject: *Re: [PATCH 2/3] ArmPlatformPkg: PrePeiCore: write early hello
> message to the serial port
>
>
>
> Hi Sami,
>
> Thank you for pointing to that patch. It pretty much does the same thing
> and I would be OK with using it instead of these changes here. Except
> that the message only shows up in debug mode and seems to depend on the
> debug mask. It would be nice if it would show up all the time.
>
> We are building ArmVirt in debug, but with
> DEBUG_PRINT_ERROR_LEVEL=0x80000000, aka a "silent" debug build, and need
> the message there too.
>
> What was proposed in this series prints the message independently of the
> debug mask.
>
>
> Regards,
>  Oliver
>
>
>
> On Tue, Jul 26, 2022 at 12:42 PM Sami Mujawar <Sami.Mujawar@arm.com>
> wrote:
>
> Hi Oliver,
>
> There is a patch for review at
> https://edk2.groups.io/g/devel/message/88884 which prints the firmware
> version string. I believe with Ard's comments addressed that patch can be
> merged.
> Would that patch solve your purpose to print an early message for debug
> purpose?
>
> Regards,
>
> Sami Mujawar
>
> On 26/07/2022, 08:29, "Oliver Steffen" <osteffen@redhat.com> wrote:
>
>     From: Laszlo Ersek <lersek@redhat.com>
>
>     Print the early hello message to the serial port.
>
>     The FixedPcdGetSize() macro expands to an integer constant, therefore
> an
>     optimizing compiler can eliminate the new code, if the platform DSC
>     doesn't override the empty string (size=1) default of
>     PcdEarlyHelloMessage.
>
>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>     Signed-off-by: Oliver Steffen <osteffen@redhat.com>
>     ---
>      ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf  | 2 ++
>      ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf | 2 ++
>      ArmPlatformPkg/PrePeiCore/PrePeiCore.h          | 1 +
>      ArmPlatformPkg/PrePeiCore/MainMPCore.c          | 7 +++++++
>      ArmPlatformPkg/PrePeiCore/MainUniCore.c         | 7 +++++++
>      5 files changed, 19 insertions(+)
>
>     diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
>     index a5b4722459d1..ea7b220bc831 100644
>     --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
>     +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
>     @@ -66,6 +66,8 @@ [FixedPcd]
>        gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
>        gArmPlatformTokenSpaceGuid.PcdCPUCoreSecondaryStackSize
>
>     +  gArmPlatformTokenSpaceGuid.PcdEarlyHelloMessage
>     +
>        gArmTokenSpaceGuid.PcdGicDistributorBase
>        gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
>        gArmTokenSpaceGuid.PcdGicSgiIntId
>     diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>     index 466a2b01c384..29fb8737cb2f 100644
>     --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>     +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>     @@ -64,4 +64,6 @@ [FixedPcd]
>        gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
>        gArmPlatformTokenSpaceGuid.PcdCPUCoreSecondaryStackSize
>
>     +  gArmPlatformTokenSpaceGuid.PcdEarlyHelloMessage
>     +
>        gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
>     diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
> b/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>     index 0345dd7bdd2a..ae8302becda2 100644
>     --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>     +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>     @@ -16,6 +16,7 @@
>      #include <Library/DebugLib.h>
>      #include <Library/IoLib.h>
>      #include <Library/PcdLib.h>
>     +#include <Library/SerialPortLib.h>
>
>      #include <PiPei.h>
>      #include <Ppi/TemporaryRamSupport.h>
>     diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
> b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
>     index b5d0d3a6442f..21c9d5f6da8f 100644
>     --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
>     +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
>     @@ -116,6 +116,13 @@ PrimaryMain (
>        UINTN                   TemporaryRamBase;
>        UINTN                   TemporaryRamSize;
>
>     +  if (FixedPcdGetSize (PcdEarlyHelloMessage) > 1) {
>     +    SerialPortWrite (
>     +      FixedPcdGetPtr (PcdEarlyHelloMessage),
>     +      FixedPcdGetSize (PcdEarlyHelloMessage) - 1
>     +      );
>     +  }
>     +
>        CreatePpiList (&PpiListSize, &PpiList);
>
>        // Enable the GIC Distributor
>     diff --git a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
> b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
>     index 1c2580eb923b..37560540e14f 100644
>     --- a/ArmPlatformPkg/PrePeiCore/MainUniCore.c
>     +++ b/ArmPlatformPkg/PrePeiCore/MainUniCore.c
>     @@ -29,6 +29,13 @@ PrimaryMain (
>        UINTN                   TemporaryRamBase;
>        UINTN                   TemporaryRamSize;
>
>     +  if (FixedPcdGetSize (PcdEarlyHelloMessage) > 1) {
>     +    SerialPortWrite (
>     +      FixedPcdGetPtr (PcdEarlyHelloMessage),
>     +      FixedPcdGetSize (PcdEarlyHelloMessage) - 1
>     +      );
>     +  }
>     +
>        CreatePpiList (&PpiListSize, &PpiList);
>
>        // Adjust the Temporary Ram as the new Ppi List (Common + Platform
> Ppi Lists) is created at
>     --
>     2.37.1
>
>
> 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.
>
>

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