[edk2-devel] Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]
Michael D Kinney
michael.d.kinney at intel.com
Fri Jan 6 03:00:54 UTC 2023
Hi Gua,
I have some general feedback on this PR. I think once this feedback is integrated you can break the commit up into a patch series for edk2 email review.
1. Adding submodule for https://github.com/MIPI-Alliance/public-mipi-sys-t.git
* .gitmodules change looks correct
* .pytool change looks correct
* Please add this external dependency to this section of edk2/ReadMe.rst
i. https://github.com/tianocore/edk2#license-details
ii. Please include edk2 stewards in review of ReadMe.rst to accept the new submodule and its license which is BSD 3 clause.
1. I am confused in the scope of the TraceHubDebugLibSysT library instance. By name it is not tied to Universal Payload, but the implementation of this lib references to Universal Payload structures.
* Could TraceHubDebugLibSysT be used in non Universal Payload platforms?
* Can Universal Payload concepts be removed from TraceHubDebugLibSysT and Universal Payload elements handled by a component in UefiPayloadPkg?
* If they can not be separate, shouldn’t all this content be moved to the UefiPayloadPkg?
2. MdeModulePkg\Include\UniversalPayload\TraceHubDebugInfo.h
* Should not include <Uefi.h> in these support includes. The module that includes this file will include top level include file like <Uefi.h> first.
* Why is pragma pack(1) used? Most UEFI/PI spec includes do not use pragma pack(1).
3. MdeModulePkg\Include\Library\TraceHubDebugLib.h
* Should not include <Uefi/UefiBaseTypes.h>. Module that uses this lib will include Uefi.h or PiDxe.h or PiPei.h that will get this include.
* Why is #include <Guid/StatusCodeDataTypeDebug.h> included in this lib class. I do not see any definitions from that .h file used in the lib class iself. Recommend remove.
* Why do TraceHubWriteCataLog64() and TraceHubWriteCataLog64StatusCode() have a return type of VOID. Can’t these fail due to bad input params or HW issues? Should they have a return type of EFI_STATUS or BOOLEAN?
* TraceHubDebugWrite(). Should it define EFI_INVALID_PARAMETER if Buffer is NULL or NumberOfBytes is 0? I see ASSERT() for NumberOfBytes 0, and I see return EFI_SUCCESS for NumberOfBytes 0. Which is the correct behavior?
4. Split mipi and mipi support files out from TraceHubDebugLibSysT library instance
* The mipi sub module provides the mipi-sys-t APIs that may be used by other lib instances in the future. By breaking it out into its own lib class/instance, it allows these mipi services to be used by more than once consumer. This is the same pattern that has been followed for other submodules such as OpenSSL, Cmocka, GoogleTest, etc.
* Add MipiSysTLib class to MdeModulePkg.dec
* MipiSysTLib class .h file should provide all the mipi related include files to support use of any of the mipi-sys-t APIs.
* TraceHubDebugLibSysT will list the MipiSysTLib in its [LibraryClasses] section to use the mipi-sys-t APIs.
5. Please explain difference in features between BaseTraceHubLib and DxeSmmTraceHubLib and PeiTraceHubLib
* If BaseTraceHubLib can be used by any module type, why do we need DxeSmmTraceHubLib or PeiTraceHubLib?
* These features differences should be summarized in the INF file headers.
6. Looks like it would be a good idea to have an internal worker function to convert GUID value from little endan to big endian that can be shared.
7. I see use of AllocateRuntimePool(). Is RtData really required and the data need to be shared with OS after ExitBootServices()?
8. PcdTraceHubDebugVerbosity
* I see use of BIT0, BIT1, BIT2 when looking at the PCD settings. I recommend you add defines/enums for these values to make it easier to understand the sources. Or perhaps add a structure with bitfields to access enable and debug level values.
* This PCD is a mix of a bit mask for the enable and an enum for the debug level. Could this be split into 2 PCDs. One for enable and one for verbosity?
9. TraceHubDebugLibSystPlatform.c is using *(UINT32 *) to write MMIO register. These must be changed to use IoLib Mmio services
10. All internal worker functions of TraceHubDebugLibSysT should remove EFIAPI and be static.
Thanks,
Mike
From: Guo, Gua <gua.guo at intel.com>
Sent: Monday, January 2, 2023 6:32 PM
To: Hsu, VictorX <victorx.hsu at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; devel at edk2.groups.io; Chan, Laura <laura.chan at intel.com>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan at intel.com>; K N, Karthik <karthik.k.n at intel.com>
Cc: Lu, James <james.lu at intel.com>; Chen, Arthur G <arthur.g.chen at intel.com>; Chiu, Chasel <chasel.chiu at intel.com>; Zimmer, Vincent <vincent.zimmer at intel.com>
Subject: RE: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]
@Chan, Laura<mailto:laura.chan at intel.com>, @Prakashan, Krishnadas Veliyathuparambil<mailto:krishnadas.veliyathuparambil.prakashan at intel.com> and @K N, Karthik<mailto:karthik.k.n at intel.com>
In parallel, please also help to review PR: https://github.com/tianocore/edk2/pull/3793 from TraceHubDebugLib owner viewpoint.
TraceHubDebugLib.h API Change
Origin
firmware.boot.uefi.iafw.intel/TraceHubDebugLib.h at main · intel-restricted/firmware.boot.uefi.iafw.intel · GitHub<https://github.com/intel-restricted/firmware.boot.uefi.iafw.intel/blob/main/ClientOneSiliconPkg/Include/Library/TraceHubDebugLib.h>
* TraceHubWriteCataLog64_0 (SeverityType, Id, )
* TraceHubWriteCataLog64_1 (SeverityType, Id, Param1, Param2)
* TraceHubWriteCataLog64_2 (SeverityType, Id, Param1, Param2)
* TraceHubWriteCataLog64_3 (SeverityType, Id, Param1, Param2, Param3)
* TraceHubWriteCataLog64_4 (SeverityType, Id, Param1, Param2, Param3, Param4)
* TraceHubWriteCataLog64_5 (SeverityType, Id, Param1, Param2, Param3, Param4, Param5)
* TraceHubWriteCataLog64_6 (SeverityType, Id, Param1, Param2, Param3, Param4, Param5, Param6)
Modify
edk2_EXP/TraceHubDebugLib.h at th_leverageMIPI_v2 · hsuc1x/edk2_EXP · GitHub<https://github.com/hsuc1x/edk2_EXP/blob/th_leverageMIPI_v2/MdeModulePkg/Include/Library/TraceHubDebugLib.h>
* TraceHubWriteCataLog64 (SeverityType, Id, 0)
* TraceHubWriteCataLog64 (SeverityType, Id, 1, Param1)
* TraceHubWriteCataLog64 (SeverityType, Id, 2, Param1, Param2)
* TraceHubWriteCataLog64 (SeverityType, Id, 3, Param1, Param2, Param3)
* TraceHubWriteCataLog64 (SeverityType, Id, 4, Param1, Param2, Param3, Param4)
* TraceHubWriteCataLog64 (SeverityType, Id, 5, Param1, Param2, Param3, Param4, Param5)
* TraceHubWriteCataLog64 (SeverityType, Id, 6, Param1, Param2, Param3, Param4, Param5, Param6)
Thanks,
Gua
From: Hsu, VictorX <victorx.hsu at intel.com<mailto:victorx.hsu at intel.com>>
Sent: Wednesday, December 28, 2022 5:51 PM
To: Guo, Gua <gua.guo at intel.com<mailto:gua.guo at intel.com>>; Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; Gao, Liming <gaoliming at byosoft.com.cn<mailto:gaoliming at byosoft.com.cn>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Chan, Laura <laura.chan at intel.com<mailto:laura.chan at intel.com>>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan at intel.com<mailto:krishnadas.veliyathuparambil.prakashan at intel.com>>; K N, Karthik <karthik.k.n at intel.com<mailto:karthik.k.n at intel.com>>; Lu, James <james.lu at intel.com<mailto:james.lu at intel.com>>; Chen, Arthur G <arthur.g.chen at intel.com<mailto:arthur.g.chen at intel.com>>; Chiu, Chasel <chasel.chiu at intel.com<mailto:chasel.chiu at intel.com>>; Zimmer, Vincent <vincent.zimmer at intel.com<mailto:vincent.zimmer at intel.com>>
Subject: RE: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]
Hi @Kinney, Michael D<mailto:michael.d.kinney at intel.com>
Please review https://github.com/tianocore/edk2/pull/3793 for below questions.
1. Need to add TraceHub PCD into edk2/MdeModulePkg.uni at master · tianocore/edk2 · GitHub<https://github.com/tianocore/edk2/blob/master/MdeModulePkg/MdeModulePkg.uni>
Answer: Done
1. What about the license ?
Answer: It’s BSD license, based on public-mipi-sys-t/LICENSE at master · MIPI-Alliance/public-mipi-sys-t (github.com)<https://github.com/MIPI-Alliance/public-mipi-sys-t/blob/master/LICENSE>
1. Is PcdTraceHubDebugAddress MMIO address ? If yes, why we use UINT32
Answer: Done
1. For TraceHubWriteCataLog64_[0-6] try to use below macro to implement it.
#define VA_START va_start
#define VA_ARG va_arg
#define VA_END va_end
#define VA_LIST va_list
Answer: Done
1. Is have SMM or MM version ?
Answer: Done, renamed to DxeSmmTraceHubDebugLibSyst.inf.
1. Whether have any chance to use .gitsubmodule to leverage https://github.com/MIPI-Alliance/public-mipi-sys-t.
Answer:. https://github.com/tianocore/edk2/pull/3793
thanks
From: Guo, Gua <gua.guo at intel.com<mailto:gua.guo at intel.com>>
Sent: Thursday, December 1, 2022 11:29 AM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; Gao, Liming <gaoliming at byosoft.com.cn<mailto:gaoliming at byosoft.com.cn>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Chan, Laura <laura.chan at intel.com<mailto:laura.chan at intel.com>>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan at intel.com<mailto:krishnadas.veliyathuparambil.prakashan at intel.com>>; K N, Karthik <karthik.k.n at intel.com<mailto:karthik.k.n at intel.com>>; Lu, James <james.lu at intel.com<mailto:james.lu at intel.com>>; Chen, Arthur G <arthur.g.chen at intel.com<mailto:arthur.g.chen at intel.com>>; Chiu, Chasel <chasel.chiu at intel.com<mailto:chasel.chiu at intel.com>>; Zimmer, Vincent <vincent.zimmer at intel.com<mailto:vincent.zimmer at intel.com>>; Hsu, VictorX <victorx.hsu at intel.com<mailto:victorx.hsu at intel.com>>
Subject: RE: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]
@Kinney, Michael D<mailto:michael.d.kinney at intel.com>
As we offline sync, there are below opens like below. Have any open, please also share for us.
1. Need to add TraceHub PCD into edk2/MdeModulePkg.uni at master · tianocore/edk2 · GitHub<https://github.com/tianocore/edk2/blob/master/MdeModulePkg/MdeModulePkg.uni>
Answer: Will update it in next patch
1. What about the license ?
Answer: It’s BSD license, based on public-mipi-sys-t/LICENSE at master · MIPI-Alliance/public-mipi-sys-t (github.com)<https://github.com/MIPI-Alliance/public-mipi-sys-t/blob/master/LICENSE>
1. Is PcdTraceHubDebugAddress MMIO address ? If yes, why we use UINT32
Answer: It’s MMIO address, we can change it to UINT64, it make sense to let the PCH more generic. Will update it in next patch.
1. For TraceHubWriteCataLog64_[0-6] try to use below macro to implement it.
#define VA_START va_start
#define VA_ARG va_arg
#define VA_END va_end
#define VA_LIST va_list
Answer: Will update it in next patch
1. Is have SMM or MM version ?
Answer: MdeModulePkg/Library/TraceHubDebugLibSysT/DxeTraceHubDebugLibSyst.inf can take care for SMM case. We can rename it like below to make it more clearly.
MdeModulePkg/Library/TraceHubDebugLibSysT/DxeSmmTraceHubDebugLibSyst.inf
1. Whether have any chance to use .gitsubmodule to leverage https://github.com/MIPI-Alliance/public-mipi-sys-t.
Answer: Will draft another PR and try to use gitsubmodule to check whether exist any predicament.
Thanks
Gua
From: Guo, Gua
Sent: Wednesday, November 23, 2022 3:29 PM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; gaoliming <gaoliming at byosoft.com.cn<mailto:gaoliming at byosoft.com.cn>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Chan, Laura <laura.chan at intel.com<mailto:laura.chan at intel.com>>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan at intel.com<mailto:krishnadas.veliyathuparambil.prakashan at intel.com>>; K N, Karthik <karthik.k.n at intel.com<mailto:karthik.k.n at intel.com>>; Lu, James <james.lu at intel.com<mailto:james.lu at intel.com>>; Chen, Arthur G <arthur.g.chen at intel.com<mailto:arthur.g.chen at intel.com>>; Chiu, Chasel <chasel.chiu at intel.com<mailto:chasel.chiu at intel.com>>; Zimmer, Vincent <vincent.zimmer at intel.com<mailto:vincent.zimmer at intel.com>>; Hsu, VictorX <victorx.hsu at intel.com<mailto:victorx.hsu at intel.com>>
Subject: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]
@Kinney, Michael D<mailto:michael.d.kinney at intel.com> and @gaoliming<mailto:gaoliming at byosoft.com.cn>
We want to add TraceHubDebug library support on Edk2. It’s new added library on Edk2, so it doesn’t have any backward and forward issue.
May I know have any process we may need to follow up. Have any comment or concern please also share for me.
Reviewers/Maintainers: @Prakashan, Krishnadas Veliyathuparambil<mailto:krishnadas.veliyathuparambil.prakashan at intel.com>, @Chan, Laura<mailto:laura.chan at intel.com>, @K N, Karthik<mailto:karthik.k.n at intel.com>.
USF UPL spec update: https://github.com/UniversalScalableFirmware/documentation/pull/52 (Done)
Edk2 Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4144 (On-Going)
Edk2 PR: https://github.com/tianocore/edk2/pull/3613 (On-Going)
Background:
* Problem: In the UPL (Universal Payload) binary which contains only the Edk2 drivers, Tracehub debug message are missing during the execution of early DXE Core modules (before ReportStatusCode is ready).
* Solution: Add TraceHub lib on Edk2, to be consumed by the UPL.
About TraceHubDebugLibSysT:
* Library is consumed by the DebugLib and StatusCodeHandler to send the PostCodes, ProgressCodes and DEBUG messages to the TraceHub/NPK in MIPI SyS-T format.
* The MIPI SyS-T APIs in this library are taken from the OpenSource implementation: https://github.com/MIPI-Alliance/public-mipi-sys-t.
* Library changes done for upstreaming: MipiSystLib is rewritten to comply EDK2 coding standards and changed ways to pass trace address/verbosity with HOB directly instead of calling into TraceHubHookLib to get these info.
* Proposal With UPL: consume HOB gUniversalPayloadTraceHubDebugInfoGuid to print TraceHub Debug message in DXE Core
Thanks,
Gua
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98054): https://edk2.groups.io/g/devel/message/98054
Mute This Topic: https://groups.io/mt/95213711/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20230106/cd2d3b9b/attachment-0001.htm>
More information about the edk2-devel-archive
mailing list