Re: [edk2] [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message
On 02/20/18 00:30, Andrew Fish wrote:
>
>
>> 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:
+ 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 0918
>>>
>>> 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.
I think a DebugLib instance located at
MdePkg/Library/DxeRuntimeDebugLibSerialPort
could be general enough, since it would not share hardware with the OS
-- it would stop runtime DXE drivers from making SerialPortLib calls.
> 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!"));
> }
Right, so this supports Ard's original idea, namely that we should
disable the DEBUGs in the client code, one way or another...
> and
>
> if (!EfiAtRuntime ()) {
> ASSERT (FALSE);
> }
... On the other hand, I think only the debug message should be
suppressed for ASSERTs; the exception or deadloop (whatever the assert
disposition) should not be suppressed at runtime. If the firmware
encounters a fatal unexpected error, it's better to hang the system
(with the deadloop) or crash it (raise an exception and make the kernel
panic) than silently corrupt more state and pretend everything's fine.
So wrapping "ASSERT (Predicate)" with "if (!EfiAtRuntime ())" does not
seem like the best solution to me.
> 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.
A "winmodem" on steroids :)
Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message
> 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 0918
>>
>> 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"
>
Re: [edk2] [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message
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 0918
>
> 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
pag
Re: [edk2] [PATCH v2 05/10] MdeModulePkg/ResetSystemRuntimeDxe: Add more debug message
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 0918
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.
Thanks,
Ard.
(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 = 0x0006
[4.234235] CM = 0, WnR = 0
[4.235178] user pgtable: 4k pages, 48-bit VAs, pgd = 80003aa97000
[4.237687] [0918] *pgd=
[4.239807] Internal error: Oops: 9606 [#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: 80003a9cd400 task.stack: 0cfe8000
[4.251350] PC is at 0x242701fc
[4.252903] LR is at 0x242701d4
[4.254054] pc : [<242701fc>] lr : [<242701d4>]
pstate: 41c5
[4.256417] sp : 0cfeba60
[4.257444] x29: 0cfeba60 x28: 80003a9cd400
[4.259094] x27: 089e1000 x26: 008e
[4.260729] x25: 0124 x24:
[4.262379] x23: x22: 0918
[4.264017] x21: 0900 x20: 0cfebb39
[4.265661] x1
