From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Liming Gao <liming.gao@intel.com>
Subject: Re: [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message
Date: Mon, 19 Feb 2018 20:23:59 +0100 [thread overview]
Message-ID: <9b3701e7-9dca-5f1f-8ce5-fbe8ab0f0ebe@redhat.com> (raw)
In-Reply-To: <CAKv+Gu9wcipJ=QoFF_GN=FRD9mqkhYn1YBCnsfdEoPbZByJdgA@mail.gmail.com>
On 02/19/18 19:59, Ard Biesheuvel wrote:
> On 9 February 2018 at 04:16, Ruiyu Ni <ruiyu.ni@intel.com> wrote:
>> The patch adds more debug message in ResetSystem().
>> It also removes unnecessary check of mResetNotifyDepth.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Reviewed-by: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> ---
>> .../Universal/ResetSystemRuntimeDxe/ResetSystem.c | 88 +++++++++++-----------
>> 1 file changed, 44 insertions(+), 44 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c b/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c
>> index fed527fac2..2c795426f5 100644
>> --- a/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c
>> +++ b/MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c
>> @@ -15,6 +15,10 @@
>>
>> #include "ResetSystem.h"
>>
>> +GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 *mResetTypeStr[] = {
>> + L"Cold", L"Warm", L"Shutdown", L"PlatformSpecific"
>> +};
>> +
>> //
>> // The current ResetSystem() notification recursion depth
>> //
>> @@ -251,16 +255,6 @@ ResetSystem (
>> LIST_ENTRY *Link;
>> RESET_NOTIFY_ENTRY *Entry;
>>
>> - //
>> - // Above the maximum recursion depth, so do the smallest amount of
>> - // work to perform a cold reset.
>> - //
>> - if (mResetNotifyDepth >= MAX_RESET_NOTIFY_DEPTH) {
>> - ResetCold ();
>> - ASSERT (FALSE);
>> - return;
>> - }
>> -
>> //
>> // Only do REPORT_STATUS_CODE() on first call to ResetSystem()
>> //
>> @@ -272,40 +266,47 @@ ResetSystem (
>> }
>>
>> mResetNotifyDepth++;
>> - if (!EfiAtRuntime () && mResetNotifyDepth < MAX_RESET_NOTIFY_DEPTH) {
>> - //
>> - // Call reset notification functions registered through the
>> - // EDKII_PLATFORM_SPECIFIC_RESET_FILTER_PROTOCOL.
>> - //
>> - for ( Link = GetFirstNode (&mPlatformSpecificResetFilter.ResetNotifies)
>> - ; !IsNull (&mPlatformSpecificResetFilter.ResetNotifies, Link)
>> - ; Link = GetNextNode (&mPlatformSpecificResetFilter.ResetNotifies, Link)
>> - ) {
>> - Entry = RESET_NOTIFY_ENTRY_FROM_LINK (Link);
>> - Entry->ResetNotify (ResetType, ResetStatus, DataSize, ResetData);
>> - }
>> - //
>> - // Call reset notification functions registered through the
>> - // EFI_RESET_NOTIFICATION_PROTOCOL.
>> - //
>> - for ( Link = GetFirstNode (&mResetNotification.ResetNotifies)
>> - ; !IsNull (&mResetNotification.ResetNotifies, Link)
>> - ; Link = GetNextNode (&mResetNotification.ResetNotifies, Link)
>> - ) {
>> - Entry = RESET_NOTIFY_ENTRY_FROM_LINK (Link);
>> - Entry->ResetNotify (ResetType, ResetStatus, DataSize, ResetData);
>> - }
>> - //
>> - // call reset notification functions registered through the
>> - // EDKII_PLATFORM_SPECIFIC_RESET_HANDLER_PROTOCOL.
>> - //
>> - for ( Link = GetFirstNode (&mPlatformSpecificResetHandler.ResetNotifies)
>> - ; !IsNull (&mPlatformSpecificResetHandler.ResetNotifies, Link)
>> - ; Link = GetNextNode (&mPlatformSpecificResetHandler.ResetNotifies, Link)
>> - ) {
>> - Entry = RESET_NOTIFY_ENTRY_FROM_LINK (Link);
>> - Entry->ResetNotify (ResetType, ResetStatus, DataSize, ResetData);
>> + DEBUG ((DEBUG_INFO, "DXE ResetSystem2: Reset call depth = %d.\n", mResetNotifyDepth));
>> +
>
> This DEBUG() print is breaking system reset from the Linux OS at
> runtime in DEBUG builds.
>
> [ 4.223704] reboot: Restarting system
> [ 4.224733] Unable to handle kernel paging request at virtual
> address 09000018
>
> This is the boottime MMIO address of the UART on the QEMU mach-virt
> model, and no runtime mapping exists for it, resulting in the crash.
>
> Please ensure that DEBUG () is used with care in DXE_RUNTIME_DRIVER modules.
Not disagreeing, just asking: should we perhaps take care of this in a
new DebugLib instance, specifically for DXE runtime drivers?
"MdePkg/Library/UefiRuntimeLib" provides functions like EfiAtRuntime()
and EfiGoneVirtual(). We couldn't use UefiRuntimeLib in DebugLib,
because UefiRuntimeLib already depends on DebugLib (we can't introduce a
circular dependency). But, we could reimplement EfiAtRuntime() manually,
in order to silence all debug messages after ExitBootServices().
This would make sense also because after ExitBootServices(), the serial
port is considered "owned" by the boot loader or the OS, and the
firmware should likely not mess up whatever IO occurs there.
I guess the two possible places to implement such runtime logic would be:
- in a RuntimeDxe clone of BaseDebugLibSerialPort (i.e., commonly for
all edk2 platforms),
- in a RuntimeDxe clone of
"ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf"
(i.e., move the checking to the serial port lib level).
(This is different from OVMF / x86, because (a) there the debug data are
written to IO port 0x402, and the IO address space does not depend on
paging, (b) largely, no boot loader or OS ever are aware of the QEMU
debug port, it can be considered as owned by the firmware, always.)
Just thinking out loud.
Thanks!
Laszlo
> (remainder of the crash log below)
>
>
> [ 4.226725] Mem abort info:
> [ 4.227564] Exception class = DABT (current EL), IL = 32 bits
> [ 4.229329] SET = 0, FnV = 0
> [ 4.230433] EA = 0, S1PTW = 0
> [ 4.232230] Data abort info:
> [ 4.233125] ISV = 0, ISS = 0x00000006
> [ 4.234235] CM = 0, WnR = 0
> [ 4.235178] user pgtable: 4k pages, 48-bit VAs, pgd = ffff80003aa97000
> [ 4.237687] [0000000009000018] *pgd=0000000000000000
> [ 4.239807] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [ 4.242203] Modules linked in:
> [ 4.243557] CPU: 0 PID: 2016 Comm: reboot Not tainted
> 4.14.20-rc1-01878-gd73f37b7c835-dirty #2100
> [ 4.246771] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> [ 4.249075] task: ffff80003a9cd400 task.stack: ffff00000cfe8000
> [ 4.251350] PC is at 0x242701fc
> [ 4.252903] LR is at 0x242701d4
> [ 4.254054] pc : [<00000000242701fc>] lr : [<00000000242701d4>]
> pstate: 400001c5
> [ 4.256417] sp : ffff00000cfeba60
> [ 4.257444] x29: ffff00000cfeba60 x28: ffff80003a9cd400
> [ 4.259094] x27: ffff0000089e1000 x26: 000000000000008e
> [ 4.260729] x25: 0000000000000124 x24: 0000000000000000
> [ 4.262379] x23: 0000000000000000 x22: 0000000009000018
> [ 4.264017] x21: 0000000009000000 x20: ffff00000cfebb39
> [ 4.265661] x19: 0000000009000018 x18: 0000000000000010
> [ 4.267288] x17: 0000ffff8bebbe00 x16: ffff000008246818
> [ 4.268916] x15: 00000000000000ff x14: 0000000000000001
> [ 4.270560] x13: 0000000000000002 x12: 0000000000000002
> [ 4.272271] x11: 0000000000000000 x10: 0000000000000002
> [ 4.274010] x9 : 0000000000000000 x8 : 0000000000000002
> [ 4.275640] x7 : 0000000024272850 x6 : 0000000000000000
> [ 4.277297] x5 : 0000000000000001 x4 : 0000000000000001
> [ 4.280108] x3 : 0000000000000000 x2 : 0000000000000001
> [ 4.282505] x1 : ffff00000cfebb10 x0 : 0000000000000001
>
>
>
>> + if (mResetNotifyDepth <= MAX_RESET_NOTIFY_DEPTH) {
>> + if (!EfiAtRuntime ()) {
>> + //
>> + // Call reset notification functions registered through the
>> + // EDKII_PLATFORM_SPECIFIC_RESET_FILTER_PROTOCOL.
>> + //
>> + for ( Link = GetFirstNode (&mPlatformSpecificResetFilter.ResetNotifies)
>> + ; !IsNull (&mPlatformSpecificResetFilter.ResetNotifies, Link)
>> + ; Link = GetNextNode (&mPlatformSpecificResetFilter.ResetNotifies, Link)
>> + ) {
>> + Entry = RESET_NOTIFY_ENTRY_FROM_LINK (Link);
>> + Entry->ResetNotify (ResetType, ResetStatus, DataSize, ResetData);
>> + }
>> + //
>> + // Call reset notification functions registered through the
>> + // EFI_RESET_NOTIFICATION_PROTOCOL.
>> + //
>> + for ( Link = GetFirstNode (&mResetNotification.ResetNotifies)
>> + ; !IsNull (&mResetNotification.ResetNotifies, Link)
>> + ; Link = GetNextNode (&mResetNotification.ResetNotifies, Link)
>> + ) {
>> + Entry = RESET_NOTIFY_ENTRY_FROM_LINK (Link);
>> + Entry->ResetNotify (ResetType, ResetStatus, DataSize, ResetData);
>> + }
>> + //
>> + // call reset notification functions registered through the
>> + // EDKII_PLATFORM_SPECIFIC_RESET_HANDLER_PROTOCOL.
>> + //
>> + for ( Link = GetFirstNode (&mPlatformSpecificResetHandler.ResetNotifies)
>> + ; !IsNull (&mPlatformSpecificResetHandler.ResetNotifies, Link)
>> + ; Link = GetNextNode (&mPlatformSpecificResetHandler.ResetNotifies, Link)
>> + ) {
>> + Entry = RESET_NOTIFY_ENTRY_FROM_LINK (Link);
>> + Entry->ResetNotify (ResetType, ResetStatus, DataSize, ResetData);
>> + }
>> }
>> + } else {
>> + ASSERT (ResetType < ARRAY_SIZE (mResetTypeStr));
>> + DEBUG ((DEBUG_ERROR, "DXE ResetSystem2: Maximum reset call depth is met. Use the current reset type: %s!\n", mResetTypeStr[ResetType]));
>> }
>>
>> switch (ResetType) {
>> @@ -331,7 +332,6 @@ ResetSystem (
>> }
>>
>> ResetWarm ();
>> -
>> break;
>>
>> case EfiResetCold:
>> --
>> 2.16.1.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
next prev parent reply other threads:[~2018-02-19 19:18 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-09 4:16 [PATCH v2 00/10] Formalize the reset system core design Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 01/10] MdePkg/PeiServicesLib: Add PeiServicesResetSystem2() Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 02/10] MdeModulePkg/PeiMain: Always attempt to use Reset2 PPI first Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 03/10] MdeModulePkg/PeiMain: Cleanup whitespace in Reset.c Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 04/10] MdeModulePkg/ResetSystemRuntimeDxe: Add platform filter and handler Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message Ruiyu Ni
2018-02-19 18:59 ` Ard Biesheuvel
2018-02-19 19:23 ` Laszlo Ersek [this message]
2018-02-19 23:30 ` Andrew Fish
2018-02-20 8:53 ` Laszlo Ersek
2018-02-09 4:16 ` [PATCH v2 06/10] MdeModulePkg: Add ResetSystemLib instances that call core services Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 07/10] MdeModulePkg: Add ResetUtility librray class and BASE instance Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 08/10] MdePkg/UefiRuntimeLib: Support more module types Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 09/10] MdeModulePkg: Add ResetSystemPei PEIM Ruiyu Ni
2018-02-09 4:16 ` [PATCH v2 10/10] MdeModulePkg/ResetSystemPei: Add reset notifications in PEI Ruiyu Ni
2018-02-09 4:55 ` [PATCH v2 00/10] Formalize the reset system core design Zeng, Star
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9b3701e7-9dca-5f1f-8ce5-fbe8ab0f0ebe@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox