AdvLoggerPkg: Fix memory bucket stability

[makubacki]

Description

Right now, the PEI Core instance of AdvancedLoggerLib will always allocate the logger buffer as EfiRuntimeServicesData. This means it is not allocated into the DXE Core managed RT Services Data memory bucket. The DXE Core instance of AdvancedLoggerLib either continues to use this buffer it is provided or allocates a new EfiReservedMemoryType buffer.

This change always allocates a EfiReservedMemoryType buffer in the DXE Core instance to ensure it is allocated from the reserved memory type buckets setup by the DXE Core. The PEI Core logger buffer is updated to be EfiBootServicesData so it does not affect runtime allocations.

• Impacts functionality?
• Impacts security?
• Breaking change?
• Includes tests?
• Includes documentation?

How This Was Tested

• CI
• Boot on a virtual and physical Intel platform and confirm the logger is functional
  • Note: Has not been tested on an ARM64 platform yet

Integration Instructions

• N/A

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 178 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release/202502@fc05e0b). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...rary/AdvancedLoggerLib/DxeCore/AdvancedLoggerLib.c	0.00%	91 Missing ⚠️
...rary/AdvancedLoggerLib/Runtime/AdvancedLoggerLib.c	0.00%	48 Missing ⚠️
...g/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c	0.00%	32 Missing ⚠️
...rary/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.c	0.00%	5 Missing ⚠️
...brary/AdvancedLoggerLib/MmCore/AdvancedLoggerLib.c	0.00%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             release/202502    #799   +/-   ##
================================================
  Coverage                  ?   4.36%           
================================================
  Files                     ?      36           
  Lines                     ?    4007           
  Branches                  ?       0           
================================================
  Hits                      ?     175           
  Misses                    ?    3832           
  Partials                  ?       0           

Flag	Coverage Δ
AdvLoggerPkg	4.36% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[makubacki]

My understanding is that there are at least two additional changes likely needed.

1. Allow an option for platforms to use a static memory region throughout all of boot. This region would be platform-defined (fixed) and not dynamically allocated. This will split the incoming buffer to DXE into either a "static" and "dynamic" case. DXE will only migrate the buffer if it falls under the "dynamic" case. The case might be able to be simply detected by checked with: PcdAdvancedLoggerBase != 0.

2. In the "dynamic" buffer case, where it is migrated to a DXE allocated buffer, the MM Core AdvancedLoggerLib instance will initially start working with the Boot Services Data buffer as described in gAdvancedLoggerHobGuid, but we'd need to send the updated shared buffer to the MM Core when it is allocated by the DXE Core instance.

For others that have worked more with adv logger, is that true? Also, any quick sanity checks of the current changes (while those updates are being made) is appreciated.

[os-d]

As noted offline, I think this works, but we should only do this if we aren't doing the fixed buffer region

[makubacki]

@cfernald, @kuqin12, @os-d, this is now ready for review.

We do plan to move to a fixed runtime region allocated in PEI. This is being checked into release/202502 which is needed by current platforms. The PEI fixed buffer may or may not be something we want to risk merging into 202502 (leaning more toward not). This can simply be adjusted at that time to not perform the buffer migration. That already is controlled with PcdAdvancedLoggerFixedInRAM. Perhaps then it would be !PcdAdvancedLoggerFixedInRAM || gEfiMemoryTypeInformationGuid Resource Descriptor HOB present or similar logic. The main point is it can relatively co-exist with PEI carved out RT memory

It could also be dropped entirely at that time if all platforms based on future release branches are forced to switch to a fixed PEI RT memory region.

[os-d]

In this window it is possible that we lose logs from an interrupt, because we copy over, then set the NewLoggerInfoAddress, so we could copy, interrupt, log to old buffer, come back and set NewLoggerInfoAddress and never get the interrupt logs. Perhaps we can:

• Write LogCurrentOffset to NewBufferInfo
• Set NewLoggerInfoAddress (and probably should do that with a compare exchange to ensure something doesn't see a partial write again in the interrupt case, like we do for the LogCurrentOffset)
• Do the copy from LogBufferOffset -> OldInfo->LogCurrentOffset

Probably simpler to disable interrupts while doing this.

[makubacki]

I raised to TPL_NOTIFY during the migration code. This prevents all practical notifies from interrupting during the full migration section and complies with UEFI spec restrictions around TPL for protocol services invocation.

[os-d]

You might still lose logs from an interrupt, though. Say a timer interrupt (or whatever) fires after the copy, before the new address set, so it will log to the old log, but here we've already done the copy and so miss out on those logs. I think it would be safer to entirely disable interrupts (could do TPL_HIGH, but also messing with TPLs can also bring headaches), so could use the CpuArch protocol (definitely available by end of DXE) to disable/enable interrupts during this time period.

If we just do that for the copy and setting the new logger address, should be fine, but I guess the driver lib is relying on the protocol being produced again, right? In which case that gets a little hairier. We can say that we might miss logs in this window and that's okay, but doesn't feel very "advanced" :)

[makubacki]

• Timer notifications are registered to TPL_NOTIFY or lower.

• Timer events are queued at their trigger time and signaled when the TPL is lowered beyond their registered level. For example, a TPL_NOTIFY timer notification will not interrupt code executing at TPL_NOTIFY.

• Execution of services from another interrupt source would generally be a violation of calling restrictions if calling into UEFI sources managed by TPL.

• OnEndOfDxe() is executed at TPL_CALLBACK.

  • So, a timer notification at TPL_NOTIFY could interrupt it

• This change surrounds the actual migration section with TPL_NOTIFY so normal timer notifications are not an issue.

Going to TPL_HIGH requires reducing the guarded section (because of calling restrictions on functions called which try to raise to TPL_NOTIFY and cause TPL inversion). I'd like to keep the set of migration code in the TPL area together right now which includes some of those calling restrictions.

The more likely case may be that an asynchronous (hardware SMI) occurs during the copy, but SMIs are not disabled with normal CPU interrupts anyway.

I guess that it's not an issue to proactively disable interrupts entirely, but in that case, I will register OnEndOfDxe() at TPL_NOTIFY itself and then disable interrupts internally in the function.

[makubacki]

Thinking about that a bit more, it might make the most sense to unblock the new region, trigger a SW MMI, and then copy in MM which would have isolation against all cases.

[makubacki]

@os-d & @kuqin12, any concern with that?

[os-d]

The timer interrupt is a bad example, because you are right that will get dispatched at TPL_NOTIFY (though any logs in the core handling of the timer interrupt and queueing we would miss). For something like the ARM64 hardware interrupt protocol, an interrupt will get generated and the exception handler will directly call the handler. There's an equivalent on the x86 side if different interrupts are registered for.

Thinking about that a bit more, it might make the most sense to unblock the new region, trigger a SW MMI, and then copy in MM which would have isolation against all cases.

That makes sense to me and as you say gives greater isolation.