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

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

[Laszlo Ersek]

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;
 //
 // 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

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

[Jordan Justen]

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
> 

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

[Brijesh Singh]

On 08/02/2018 07:30 PM, 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!


I have tested the patch on SEV and it works well with and without the 
debug flag. thank you!

Tested-by: Brijesh Singh <brijesh.singh@amd.com>

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

[Laszlo Ersek]

On 08/03/18 08:42, Jordan Justen wrote:
> 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?

Yes, both variables could be defined in PlatformDebugLibIoPortFound()
now -- and that would actually be superior coding practice in any other
C-language project :)

In edk2, objects with block scope declaration, static storage duration,
and no linkage (aka "function global variables") basically don't exist.

I've sneakily added a handful of them to OvmfPkg, over time, but only so
I could attach names to string literals that I'd use multiple times. See
"Fallback" in "Library/PlatformBootManagerLib/BdsPlatform.c", and
"ConvFallBack" in "Library/QemuBootOrderLib/QemuBootOrderLib.c".

(Such variables don't get the "m" prefix though.)

> If not, should it be follow by a blank line?

In edk2 there is "prior art" for both keeping and not keeping [*] a
blank line just before a comment block. I strive to decide consciously,
and this was no exception -- I felt that the 1st and 3rd lines in the
subsequent comment

//
// Set to TRUE if the debug I/O port is enabled
//

gave enough separation.

[*] One example of the many: see near the macro and enum constant
definitions in "MdePkg/Include/Uefi/UefiMultiPhase.h".

> 
> I agree that Brijesh should verify it, but pending that:
> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thanks!

Should I resubmit the patch for function-scoping (and renaming) the
global variables, or for inserting the blank linke?

(I guess I could insert the blank line right before I push, too, if you
prefer that.)

Thanks!
Laszlo

>>  //
>>  // 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
>>

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

[Jordan Justen]

On 2018-08-03 08:08:13, Laszlo Ersek wrote:
> 
> Should I resubmit the patch for function-scoping (and renaming) the
> global variables, or for inserting the blank linke?

No need to resubmit. Furthermore, you can consider my suggestions as
optional. I don't feel too strongly about these ones.

-Jordan

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

[Laszlo Ersek]

On 08/06/18 20:26, Jordan Justen wrote:
> On 2018-08-03 08:08:13, Laszlo Ersek wrote:
>>
>> Should I resubmit the patch for function-scoping (and renaming) the
>> global variables, or for inserting the blank linke?
>
> No need to resubmit. Furthermore, you can consider my suggestions as
> optional. I don't feel too strongly about these ones.

I've traded the blank line which I superfluously added above
"mDebugIoPortChecked", initially, for the one you pointed out as
missing:

> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> index 74aef2e37b42..e24cc834c2a3 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> @@ -16,11 +16,11 @@
>  #include <Base.h>
>  #include "DebugLibDetect.h"
>
> -
>  //
>  // Set to TRUE if the debug I/O port has been checked
>  //
>  STATIC BOOLEAN mDebugIoPortChecked = FALSE;
> +
>  //
>  // Set to TRUE if the debug I/O port is enabled
>  //

I noted this on the commit message too.

Pushed as commit 91a5b1365075.

Thank you both!
Laszlo