> 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

Reply via email to