Re: [PATCH] OvmfPkg/PlatformDebugLibIoPort: fix port detection for use in the DXE Core

[Jordan Justen <jordan.l.justen@intel.com>]

On 2018-08-02 17:30:45, Laszlo Ersek wrote:
> The DXE Core is one of those modules that call
> ProcessLibraryConstructorList() manually.
> 
> Before DxeMain() [MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c] calls
> ProcessLibraryConstructorList(), and through it, our
> PlatformDebugLibIoPortConstructor() function, DxeMain() invokes the
> DEBUG() macro multiple times. That macro lands in our
> PlatformDebugLibIoPortFound() function -- which currently relies on the
> "mDebugIoPortFound" global variable that has (not yet) been set by the
> constructor. As a result, early debug messages from the DXE Core are lost.
> 
> Move the device detection into PlatformDebugLibIoPortFound(), also caching
> the fact (not just the result) of the device detection.
> 
> (We could introduce a separate DebugLib instance just for the DXE Core,
> but the above approach works for all modules that currently consume the
> PlatformDebugLibIoPort instance (which means "everything but SEC").)
> 
> This restores messages such as:
> 
> > CoreInitializeMemoryServices:
> >   BaseAddress - 0x7AF21000 Length - 0x3CDE000 MinimalMemorySizeNeeded - 0x10F4000
> 
> Keep the empty constructor function -- OVMF's DebugLib instances have
> always had constructors; we had better not upset constructor dependency
> ordering by making our instance(s) constructor-less.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Fixes: c09d9571300a089c35f5df2773b70edc25050d0d
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Brijesh, can you please test this patch on SEV, with and without
>     capturing the debug port? (In the first case, the debug log should just
>     work; in the second case, the boot should remain fast.) Thanks!
>     
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: debuglib_dxecore
> 
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c | 24 ++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> index 81c44eece95f..74aef2e37b42 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> @@ -16,14 +16,26 @@
>  #include <Base.h>
>  #include "DebugLibDetect.h"
>  
> +
> +//
> +// Set to TRUE if the debug I/O port has been checked
> +//
> +STATIC BOOLEAN mDebugIoPortChecked = FALSE;

Could this be a static variable in the function? If not, should it be
follow by a blank line?

I agree that Brijesh should verify it, but pending that:

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

>  //
>  // Set to TRUE if the debug I/O port is enabled
>  //
>  STATIC BOOLEAN mDebugIoPortFound = FALSE;
>  
>  /**
> -  This constructor function checks if the debug I/O port device is present,
> -  caching the result for later use.
> +  This constructor function must not do anything.
> +
> +  Some modules consuming this library instance, such as the DXE Core, invoke
> +  the DEBUG() macro before they explicitly call
> +  ProcessLibraryConstructorList(). Therefore the auto-generated call from
> +  ProcessLibraryConstructorList() to this constructor function may be preceded
> +  by some calls to PlatformDebugLibIoPortFound() below. Hence
> +  PlatformDebugLibIoPortFound() must not rely on anything this constructor
> +  could set up.
>  
>    @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
>  
> @@ -34,12 +46,12 @@ PlatformDebugLibIoPortConstructor (
>    VOID
>    )
>  {
> -  mDebugIoPortFound = PlatformDebugLibIoPortDetect();
>    return RETURN_SUCCESS;
>  }
>  
>  /**
> -  Return the cached result of detecting the debug I/O port device.
> +  At the first call, check if the debug I/O port device is present, and cache
> +  the result for later use. At subsequent calls, return the cached result.
>  
>    @retval TRUE   if the debug I/O port device was detected.
>    @retval FALSE  otherwise
> @@ -51,5 +63,9 @@ PlatformDebugLibIoPortFound (
>    VOID
>    )
>  {
> +  if (!mDebugIoPortChecked) {
> +    mDebugIoPortFound = PlatformDebugLibIoPortDetect ();
> +    mDebugIoPortChecked = TRUE;
> +  }
>    return mDebugIoPortFound;
>  }
> -- 
> 2.14.1.3.gb7cf6e02401b
>