From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=17.151.62.29; helo=mail-in7.apple.com; envelope-from=afish@apple.com; receiver=edk2-devel@lists.01.org Received: from mail-in7.apple.com (mail-out7.apple.com [17.151.62.29]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7C48920954CBA for ; Mon, 19 Feb 2018 15:24:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1519083051; x=2382996651; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=oIGp03k6BcKxSPMcL+XFtYJ3wZq8JBcODkXK375kYng=; b=lU6MZjisyd5bfF4bG07aPS03OxbjzI359Y1dcfgSeSoQhaxYGtT8odnyNVj7cumf FFs/JG15EALUxrBzJ8tO1WjlihS93vfmT+q60owkHUj4pe3gC51t7jZ76OGp0Ocg D4g/IVKawfwHTlLfrZOIGcMs4gfhjdy2WOoh3/q8h6MNZ3ck4kh2hg7oOhe95z76 ICrCTZl3Gh/yULqbySlYV3PhdiyFE/ZSpBeZ9C2MWfATF7T+SqaeVEQ9x4PLqUJZ LnVCsidRxSTYjrJr4cZH010wnivkMj0ntO4gw+3Ufm09Liyp209LyC3+Y3Ylydwi 1qPpMML2O65QOvESE+rIfw==; Received: from relay2.apple.com (relay2.apple.com [17.128.113.67]) (using TLS with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail-in7.apple.com (Apple Secure Mail Relay) with SMTP id 95.31.04908.B2E5B8A5; Mon, 19 Feb 2018 15:30:51 -0800 (PST) X-AuditID: 11973e16-f4dff7000000132c-aa-5a8b5e2bcd9f Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by relay2.apple.com (Apple SCV relay) with SMTP id BD.7C.26650.B2E5B8A5; Mon, 19 Feb 2018 15:30:51 -0800 (PST) MIME-version: 1.0 Received: from [17.114.153.85] by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.2.20180130 64bit (built Jan 30 2018)) with ESMTPSA id <0P4F004HM7ZEEK60@nwk-mmpp-sz09.apple.com>; Mon, 19 Feb 2018 15:30:51 -0800 (PST) Sender: afish@apple.com From: Andrew Fish Message-id: <813EF154-1100-4151-A52D-2FD80D523B5E@apple.com> Date: Mon, 19 Feb 2018 15:30:11 -0800 In-reply-to: <9b3701e7-9dca-5f1f-8ce5-fbe8ab0f0ebe@redhat.com> Cc: Ard Biesheuvel , Ruiyu Ni , Mike Kinney , "edk2-devel@lists.01.org" , Liming Gao To: Laszlo Ersek References: <20180209041635.320856-1-ruiyu.ni@intel.com> <20180209041635.320856-6-ruiyu.ni@intel.com> <9b3701e7-9dca-5f1f-8ce5-fbe8ab0f0ebe@redhat.com> X-Mailer: Apple Mail (2.3445.5.20) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrKLMWRmVeSWpSXmKPExsUi2FDorKsd1x1lsHGtnsX/D7sZLfYcOsps sezYDhaLFfc2sFt0dPxjsnjZs5rdgc1j8Z6XTB53ru1h8+ie/Y/F4/2+q2wBLFFcNimpOZll qUX6dglcGXebH7MULF7CXLGp5xlTA+ONt0xdjJwcEgImEnfeTWLrYuTiEBJYzSQxbeV/FpjE lulLmCASBxklzh69wA6S4BUQlPgx+R5YEbNAmMTeR38ZIYq+Mkq0PH0MlhAWEJd4d2YTM4jN JqAssWL+B6hmG4lVl39C1SRKXP4E0szJwSKgKvF5+xdWEJtTwE5i480/rBALrjJKrOvQALFF BFQkZk94AHXRI0aJa30XoX5Qkpj+/TbYDxICR9gkvry9wD6BUWgWkmtnIbkWwtaS+P6oFSjO AWTLSxw8LwsR1pR4du8TO4StLfHk3QXWBYxsqxiFchMzc3Qz88z1EgsKclL1kvNzNzGC4mm6 ndgOxoerrA4xCnAwKvHw7rjVFSXEmlhWXJl7iFGag0VJnHf9x84oIYH0xJLU7NTUgtSi+KLS nNTiQ4xMHJxSDYx5+ee//YpS+ml/sNxh646ihmuq927OZt4TyHpCrFxzxaNJn0IWnFxxwltk WjWTw32TtHmL9CUEozJ+/FPx4D8uq/KWOyR75Z3IQ6dENr7bFr+1u3mLyuqApbkKU557K2xq nL7p2KnC1mJv19IX7632rmfpi9xiNu3+8cDrp0qmnzplXl/x5sVqJZbijERDLeai4kQA//sF fogCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrBLMWRmVeSWpSXmKPExsUi2FAcoKsd1x1lsPO4hsX/D7sZLfYcOsps sezYDhaLFfc2sFt0dPxjsnjZs5rdgc1j8Z6XTB53ru1h8+ie/Y/F4/2+q2wBLFFcNimpOZll qUX6dglcGXebH7MULF7CXLGp5xlTA+ONt0xdjJwcEgImElumLwGyuTiEBA4ySpw9eoEdJMEr ICjxY/I9FhCbWSBMYu+jv4wQRV8ZJVqePgZLCAuIS7w7s4kZxGYTUJZYMf8DVLONxKrLP6Fq EiUufwJp5uRgEVCV+Lz9CyuIzSlgJ7Hx5h9WiAVXGSXWdWiA2CICKhKzJzyAuugRo8S1votQ pypJTP9+m20CI/8sJAfOQnIghK0l8f1RK1CcA8iWlzh4XhYirCnx7N4ndghbW+LJuwusCxjZ VjEKFKXmJFYa6SUWFOSk6iXn525iBId/ofMOxmPLrA4xCnAwKvHw7rjVFSXEmlhWXJl7iFGC g1lJhDdHpDtKiDclsbIqtSg/vqg0J7X4EKM0B4uSOO9JKaCUQHpiSWp2ampBahFMlomDU6qB sdNLqL44ie9hyHbv4iSti+/OaVVoX3NzfXZO8LuBuPnCJRzH/4VxX5yXy9BxZ3Lorc09DftY v+dskQ2QeTI1zW5+bGWjjRNj6rxTedO397VzH/cy6Vo25Vf33uvKn5wnX5Gescr1zvsyVp4c FVVxBZlA95XpAtnWto8Ek3fvcNd9+mfhtVU3lFiKMxINtZiLihMBIiuumnsCAAA= X-Content-Filtered-By: Mailman/MimeDel 2.1.23 Subject: Re: [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Feb 2018 23:24:54 -0000 Content-Type: text/plain; CHARSET=US-ASCII Content-Transfer-Encoding: 7BIT > On Feb 19, 2018, at 11:23 AM, Laszlo Ersek wrote: > > On 02/19/18 19:59, Ard Biesheuvel wrote: >> On 9 February 2018 at 04:16, Ruiyu Ni wrote: >>> The patch adds more debug message in ResetSystem(). >>> It also removes unnecessary check of mResetNotifyDepth. >>> >>> Cc: Liming Gao >>> Cc: Michael D Kinney >>> Reviewed-by: Star Zeng >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ruiyu Ni >>> --- >>> .../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. > Laszlo, >>From a Pedantic point of view an EFI Runtime Service can only use hardware not exposed to OS as there is no clean way to share. There are some scary wiggle words about the RTC that date all the way back to EFI 1.1, and that is the only conformant exception. So that is probably why we don't have a generic solution as it is kind of dangerous. For example what happens if the OS has a kernel debugger running on that serial port and EFI Spews DEBUG prints, that would probably not come out well. For things I've written I usually end up writing a macro that does something like: if (!EfiAtRuntime ()) { DEBUG ((DEBUG_ERROR, "Hello World!")); } and if (!EfiAtRuntime ()) { ASSERT (FALSE); } Maybe it would possible to add a RUNTIME_DEBUG(), RUNTIME_ASSERT(), etc. macros to the UefiRuntimeLib? Makes me remember the story from back in the 1990's about and update to the Windows Plug-N-Play subsystem to auto magically detect modems. It worked great, and made it easier to get folks online (even if it was very slow), but seems a software update managed to destroy a very very expensive custom milling machine. It seems this milling machine was connected to the serial port of the PC, and it was a very dumb device as it interpreted data across the serial port as coordinates and commands, and all the error checking was done on the PC. So these random data on the serial line told the milling machine to attack its self. Thanks, Andrew Fish > 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 >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel