MdePkg/BasePeCoffLib: Remove DEBUG() statements from runtime code

[ardbiesheuvel]

PeCoffLoaderRelocateImageForRuntime() executes after boot services, and so it should not use DEBUG() prints at all, given that these may rely on MMIO mappings or other boot time facilities that are no longer available.

So revert the changes in aedcaa3 ("MdePkg: Fix overflow issue in PeCoffLoaderRelocateImageForRuntime") that replaced code comments with DEBUG() statements.

[ardbiesheuvel]

@tlendacky @sachinami

Fix for bug introduced by #10617

[bssrikanth]

Thanks, tested this PR and it fixes the issue reported at https://edk2.groups.io/g/devel/topic/edk2_master_amd_sev_es_guest/110856145.

Reported-by: Srikanth Aithal [sraithal@amd.com]
Tested-by: Srikanth Aithal [sraithal@amd.com]

[sachinami]

Thank you @ardbiesheuvel !

[mdkinney]

DEBUG() macros can potentially be used from any environment as long as there is a DebugLib and a device that is safe to use to log the message. Removing DEBUG() messages is not the correct way to handle this. Instead, it should be a DSC configuration to set the right DebugLib library mapping and the right PCD settings to filter DEBUG() messages.

[ardbiesheuvel]

DEBUG() macros can potentially be used from any environment as long as there is a DebugLib and a device that is safe to use to log the message. Removing DEBUG() messages is not the correct way to handle this. Instead, it should be a DSC configuration to set the right DebugLib library mapping and the right PCD settings to filter DEBUG() messages.

I agree with this as a general principle. However, these DEBUG() statements were introduced only last week, caused an actual regression, are unrelated to the purpose of the patch in question, and were not documented at all in its commit log.

So reverting this part of the change is reasonable. If there is sufficient rationale for adding these DEBUG() prints, they can be added and documented in a separate patch.

[tlendacky]

Looks good.

Reviewed-by: Tom Lendacky thomas.lendacky@amd.com

[mdkinney]

I will point out that the fact this broke a specific platform means that platform is not configured correctly and is at risk of the same type of failure from every future change that may introduce a DEBUG() message in a RT component or RT dependent library. You can use the change that added the DEBUG() message as a way to test if the platform configuration has been appropriately addressed.

[leiflindholm]

I will point out that the fact this broke a specific platform means that platform is not configured correctly and is at risk of the same type of failure from every future change that may introduce a DEBUG() message in a RT component or RT dependent library. You can use the change that added the DEBUG() message as a way to test if the platform configuration has been appropriately addressed.

This is all true, but I guarantee this is not something that every other platform in existence gets right. This is a very sharp corner in the codebase. To the point where I wonder if we shouldn't consider a separate macro for DEBUG messages intended to be output during runtime to act as a bit of padding (and perhaps a route towards build-time detection of misconfiguration instead of unexpected runtime detection).

[mdkinney]

A different macro will not work. There are many libs of type BASE that are compatible with both RT and non RT environments.

Since this specific change is to remove DEBUG() messages added to an area where they were not present before, I will approve and merge this one change.

I recommend that this discussion of how to detect incorrectly configured platforms for RT debug messages be moved to a new
GitHub Issue.

[ardbiesheuvel]

Thanks.