[Bug]: AdvancedLoggerHdwPortLib should not check PCD for header versions greater than 2

[spbrogan]

Is there an existing issue for this?

• I have searched existing issues

Current Behavior

wasted execution time on a hot path comparing the debug level against the PCD at both levels.

mu_plus/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.c

Line 64 in a3d53ef

if (DebugLevel & PcdGet32 (PcdAdvancedLoggerHdwPortDebugPrintErrorLevel)) {

Why does calling code just mask the bits rather than avoid the function call?

mu_plus/AdvLoggerPkg/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c

Line 148 in a3d53ef

DebugLevel = (DebugLevel & LoggerInfo->HwPrintLevel);

Expected Behavior

Expected to be as efficient as possible and require minimal code in lower layers.

Steps To Reproduce

Look at code

Build Environment

NA

Version Information

a3d53ef6296bc0b1309cf2654017eb394a726dd4

all versions.

Urgency

Medium

Are you going to fix this?

Someone else needs to fix it

Do you need maintainer feedback?

No maintainer feedback needed

Anything else?

May have impact on downstream/out-of-tree projects and their implementations.

[os-d]

There is a case here where we could not have LoggerInfo, in the SEC instance:

mu_plus/AdvLoggerPkg/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c

Lines 141 to 153 in a3d53ef

	#ifdef ADVANCED_LOGGER_SEC
	// If LoggerInfo == NULL, assume there is a HdwPort and it has not been disabled. This
	// does occur in SEC
	if ((LoggerInfo == NULL) || (!LoggerInfo->HdwPortDisabled)) {
	#else
	if ((LoggerInfo != NULL) && (!LoggerInfo->HdwPortDisabled)) {
	if (LoggerInfo->Version >= ADVANCED_LOGGER_HW_LVL_VER) {
	DebugLevel = (DebugLevel & LoggerInfo->HwPrintLevel);
	}
	#endif
	AdvancedLoggerHdwPortWrite (DebugLevel, (UINT8 *)Buffer, NumberOfBytes);
	}

.

So, either we need the HdwPortLib to check the PCD and the upper lib not to, or the SEC instance needs to be reworked or have a separate code path. The smaller delta here is having the HdwPortLib check the PCD and being unconditionally called, so I'll put up a PR for that and we can discuss if a different approach is better.

The other consideration here, though, is who will be implementing HdwPortLib. If this is intended to only use the mu_plus version, great, we can drop the upper layer's checking of the PCD. If a platform may implement HdwPortLib, then the core relies on the platform to check the PCD to only log what fields are set, which is a platform level decision, but may or may not be the intention of the AdvLogger flow.

[os-d]

Looking again, if we unconditionally log in SEC, which seems reasonable, we can drop this double checking.

[kuqin12]

There were different use cases for the bitmask vs. PCD. The bitmask was added for inheriting the level set by its loader (i.e. PEI core, when DXE is loaded, etc.). But the PCD was left alone to keep the functionality where the print level for each module can be changed on demand.