> On Mar 13, 2019, at 1:16 PM, Brian J. Johnson <brian.john...@hpe.com> wrote:
>
> On 3/12/19 12:28 PM, Andrew Fish via edk2-devel wrote:
>>> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>>>
>>> Hello Heyi,
>>>
>>> On 03/12/19 07:56, Heyi Guo wrote:
>>>> Hi Laszlo,
>>>>
>>>> I'm thinking about a proposal as below:
>>>>
>>>> 1. Reuse the framework of
>>>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>>>>
>>>> 2. The boot time behavior of this module is not changed
>>>> 3. Switch to status code protocol for runtime debug
>>>> 4. Use an overridden SerialPortLib instance for
>>>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>>>>
>>>> Then we can have below benefits:
>>>> 1. the boot time firmware log, and the OS log, went to one port; and
>>>> there is no dependence for runtime drivers to output serial message at
>>>> boot time.
>>>> 2. By implementing different instances of SerialPortLib for
>>>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>>>> runtime serial message to platform specific serial port.
>>>
>>> This looks a bit over-engineered to me. Ultimately our goal is:
>>> - for DEBUGs to reach "a" serial port at runtime -- namely one that
>>> differs from the "normal" serial port.
>>>
>>> Given that you'd have to implement a "special" SerialPortLib instance
>>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>>
>>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>>
>>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>>> (a) runtime behavior and (b) handling of a special serial port?
>>>
>>> In other words, push down the "runtime?" distinction from DebugLib (see
>>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>>> would be used only with DXE_RUNTIME_DRIVERs.
>>>
>>> The lib instance would prepare everything in advance for accessing the
>>> "special" serial port at runtime (which could require allocating runtime
>>> memory in advance). Once ExitBootServices() is called, a callback should
>>> switch over the behavior of the lib instance, without allocating further
>>> memory. (The switched-over behavior would not have to be functional
>>> until the virtual address map is set.)
>>>
>> Laszlo,
>> Runtime does not quite work like that. As soon as the Set Virtual Address
>> map fires it is only legal to call things EFI RT from the virtual mapping.
>> The last stage of the Set Virtual Address Map call is to reapply the fixups
>> in the Runtime Drivers for the virtual address space. That means any
>> absolute address reference in the code now points to the virtual address.
>> Also the Set Virtual Address event told the Runtime Driver to convert all
>> its pointers to point to the new virtual address space.
>> The blackout and the fact you need to hide the hardware from the OS make
>> doing things at EFI Runtime Time hard.
>> I agree with you that since our logging is really just a serial stream we
>> don't require Report Status Code. Report Status Code exists so that the old
>> PC POST Cards could still be supported, see:
>> https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is
>> also a Report Status Code layer that allows redirecting the stream to
>> multiple agents, so for example you could send data to port 80 (POST Card)
>> and write out the same values in the serial stream. I find lookup up Report
>> Status Code codes very tedious and not really helpful for debugging vs.
>> DEBUG prints.
>> Thanks,
>> Andrew Fish
>
> Andrew, wasn't Report Status Code also intended to reduce code size?
> PeiDxeDebugLibReportStatusCode packages up DEBUG() arguments into a structure
> and passes it to the status code handler. It avoids having to link a
> PrintLib and SerialPortLib instance into essentially every module. At least
> that was the intent... IIRC at some point a few years ago folks realized that
> VA_LIST wasn't portable across compilers, so now
> PeiDxeDebugLibReportStatusCode converts VA_LIST into BASE_LIST, which
> requires parsing the format string much like PrintLib does. No idea if that
> actually results in a space savings over BaseDebugLibSerialPort, especially
> for really simple SerialPortLib instances.
>
Brian,
Good point about size. We are just talking about runtime drivers, and they are
likely all compressed so it would be interesting to know the impact.
> +1 for the difficulty of decoding Report Status Code codes... there has to be
> a better way to do that than manually parsing through PiStatusCode.h and
> friends.
>
I seem to remember there was some code that shortened it to a post code so the
common codes ended up being a byte. But it seems like post processing tools
would be useful.
Thanks,
Andrew Fish
> Brian Johnson
>
>>> I've always seen status code reporting cumbersome in comparison to plain
>>> DEBUG messages. My understanding is that status code reporting targets
>>> devices that are even dumber than serial ports. But, we do target a
>>> second serial port. So if we can keep the switchover internal to the
>>> SerialPortLib class, at least for runtime DXE drivers, then I think we
>>> should do that.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>>>> +Peter, for the last few paragraphs
>>>>>
>>>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>>>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoh...@huawei.com> wrote:
>>>>>>> Serial port output is useful when debugging UEFI runtime services in
>>>>>>> OS runtime.
>>>>>>> The patches are trying to provide a handy method to enable runtime
>>>>>>> serial port
>>>>>>> debug for ArmVirtQemu.
>>>>>>>
>>>>>>> Cc: Jian J Wang <jian.j.w...@intel.com>
>>>>>>> Cc: Hao Wu <hao.a...@intel.com>
>>>>>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>>>>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>>>>>> Cc: Julien Grall <julien.gr...@arm.com>
>>>>>>>
>>>>>>> Heyi Guo (3):
>>>>>>> MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>>>> ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>>>> ArmVirtQemu: enable runtime debug by build flag
>>>>>>>
>>>>>> Hello Heyi,
>>>>>>
>>>>>> We have had this discussion before, and IIRC, the proposed solution
>>>>>> was to use status codes.
>>>>>>
>>>>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>>>>> one of the MdeModulePkg maintainers can elaborate.
>>>>> Here's the basic idea.
>>>>>
>>>>> Status Code reporting and routing are defined by the PI spec for OS
>>>>> runtime as well, not just boot time.
>>>>>
>>>>> Recently we added status code *routing* to ArmVirtPkg, in commit
>>>>> 5c574b222ec2, via the generic runtime driver
>>>>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>>>>
>>>>> ReportStatusCodeLib -->
>>>>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>>
>>>>> (for some module types). As a result, when modules of those types report
>>>>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>>>>> driver, which "routes" the status codes.
>>>>>
>>>>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>>>>> (built into BdsDxe) to register a status code *handler* callback with
>>>>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>>>>> runtime), and only for a very specific group of status codes. As a
>>>>> result, when a module of suitable type reports a status code,
>>>>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>>>>> "handles it" (in our implementation, we simply format it to the UEFI
>>>>> console), assuming the status code is the kind we are interested in.
>>>>>
>>>>>
>>>>> Now we come to the current series. First, the series adds the following
>>>>> DebugLib class resolution:
>>>>>
>>>>> DebugLib -->
>>>>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>>>>
>>>>>
>>>>> This will cause modules to publish their DEBUG messages as status codes
>>>>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>>>>> too lazy to check the exact status code classes etc that
>>>>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>>>>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>>>>> "routing". As another result, until we reach a late enough stage in the
>>>>> boot, those messages will not be printed by anything (because there's
>>>>> not going to be any "handling" for them).
>>>>>
>>>>> (The present series also updates the ReportStatusCodeLib resolution so
>>>>> it can be used at runtime too, but that's quite auxiliary to this
>>>>> discussion.)
>>>>>
>>>>> Second, this series includes the generic Status Code *handling* driver
>>>>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>>>>> the particular handling that we already have in BdsDxe via the earlier
>>>>> series, this generic status handler driver registers a handler callback
>>>>> that simply prints status codes to the serial port (dependent on a PCD
>>>>> setting), via SerialPortLib.
>>>>>
>>>>> With the modification from the first patch, this generic status code
>>>>> *handler* driver is extended to runtime serial port operation. And, the
>>>>> second patch in the series provides a SerialPortLib instance for it that
>>>>> can work at runtime.
>>>>>
>>>>> All in all, when a runtime driver calls DEBUG, this happens:
>>>>>
>>>>> runtime driver calls DEBUG
>>>>> -> PeiDxeDebugLibReportStatusCode
>>>>> -> RuntimeDxeReportStatusCodeLib
>>>>> [status code reporting]
>>>>>
>>>>> => ReportStatusCodeRouterRuntimeDxe
>>>>> [status code routing]
>>>>>
>>>>> => StatusCodeHandlerRuntimeDxe
>>>>> [status code handling, such as SerialPortWrite():]
>>>>> -> FdtPL011SerialPortLibRuntime
>>>>>
>>>>> This is actually a good idea, but it would be nice if:
>>>>> - QEMU had two PL011 ports,
>>>>> - the boot time firmware log, and the OS log, went to one port
>>>>> - the runtme firmware log went to the other port.
>>>>>
>>>>> Given that this series provides the SerialPortLib instance for
>>>>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>>>>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>>>>> for runtime firmware logging.
>>>>>
>>>>> I don't insist that this series implement all of that, but it should
>>>>> either prevent a conflict on the one PL011 between the firmware and the
>>>>> OS, or else be very explicit about the possible conflict in the commit
>>>>> messages.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>> _______________________________________________
>>> 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