Re: [PATCH 1/2] OvmfPkg: create a separate PlatformDebugLibIoPort instance for SEC

[Laszlo Ersek <lersek@redhat.com>]

On 11/16/17 11:47, Paolo Bonzini wrote:
> The next patch will want to add a global variable to
> PlatformDebugLibIoPort, but this is not suitable for the SEC
> phase, because SEC runs from read-only flash.  The solution is
> to have two library instances, one for SEC and another
> for all other firmware phases.  This patch adds the "plumbing"
> for the SEC library instance, separating the INF files and
> moving the constructor to a separate C source file.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen (Intel address) <jordan.l.justen@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c  | 15 -------
>  .../PlatformDebugLibIoPort/DebugLibDetect.c        | 32 +++++++++++++
>  .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 32 +++++++++++++
>  .../PlatformDebugLibIoPort.inf                     |  1 +
>  .../PlatformRomDebugLibIoPort.inf                  | 52 ++++++++++++++++++++++
>  OvmfPkg/OvmfPkgIa32.dsc                            |  2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                         |  2 +-
>  OvmfPkg/OvmfPkgX64.dsc                             |  2 +-
>  8 files changed, 120 insertions(+), 18 deletions(-)
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf

Very nice!

I have a number of superficial comments. I've considered squashing these changes into your patch myself, but I didn't know what you'd prefer, so first I'll just list them.

> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index 5435767c1c..a5572a8eeb 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -29,21 +29,6 @@
>  //
>  #define MAX_DEBUG_MESSAGE_LENGTH  0x100
>  
> -/**
> -  This constructor function does not have to do anything.
> -
> -  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
> -
> -**/
> -RETURN_STATUS
> -EFIAPI
> -PlatformDebugLibIoPortConstructor (
> -  VOID
> -  )
> -{
> -  return EFI_SUCCESS;
> -}
> -
>  /**
>    Prints a debug message to the debug output device if the specified error level is enabled.
>  

(1) This is a pre-existent wart in the code. The return type is RETURN_STATUS, but we return EFI_SUCCESS instead of RETURN_SUCCESS.

Numerically they are the same, but the message is different. RETURN_* is for library instances that have MODULE_TYPE=BASE, meaning that they can be built into modules that are more "primitive" than DXE_DRIVERs (such as SEC, PEIM, PEI_CORE, ...). This distinction primarily influences the constructor function's parameter list -- for MODULE_TYPE=BASE library instances, it is just VOID --, but it also dictates whether we should use EFI_xxx or RETURN_xxx as status codes.

Can you please prepend a patch to the series that replaces EFI_SUCCESS with RETURN_SUCCESS in this function?

(2) Similarly, as a "precursor cleanup", <Uefi.h> should not be included anywhere in this library instance. Namely, "MdePkg/Include/Uefi.h" has the following leading comment (rewrapped here for readability):

> Root include file for Mde Package UEFI, UEFI_APPLICATION type modules.
>
> This is the include file for any module of type UEFI and
> UEFI_APPLICATION. Uefi modules only use types defined via this include
> file and can be ported easily to any environment.

UEFI_DRIVER and UEFI_APPLICATION modules are the *least* primitive module types, while this library instance is on the other end of the spectrum.

Now, "MdePkg/Include/Uefi.h" is just a convenience header for including:

#include <Uefi/UefiBaseType.h>
#include <Uefi/UefiSpec.h>

>From these, "MdePkg/Include/Uefi/UefiBaseType.h" is totally fine to include wherever, while "MdePkg/Include/Uefi/UefiSpec.h" is the one that should be kept out of BASE lib classes.

Thus, in the same prepended patch, can you please also try to replace <Uefi.h> with <Uefi/UefiBaseType.h>? The lib instance should continue building just the same. (If it breaks, that means we have a layering violation in the code somewhere.)

Consequently:

> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> new file mode 100644
> index 0000000000..fee908861b
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> @@ -0,0 +1,32 @@
> +/** @file
> +  Constructor code for QEMU debug port library.
> +  Non-SEC instance.
> +
> +  Copyright (c) 2017, Red Hat, Inc.<BR>
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +
> +/**
> +  This constructor function does not have anything to do.
> +
> +  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PlatformDebugLibIoPortConstructor (
> +  VOID
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> new file mode 100644
> index 0000000000..407fe613ff
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> @@ -0,0 +1,32 @@
> +/** @file
> +  Constructor code for QEMU debug port library.
> +  SEC instance.
> +
> +  Copyright (c) 2017, Red Hat, Inc.<BR>
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +
> +/**
> +  This constructor function does not have to do anything.
> +
> +  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PlatformRomDebugLibIoPortConstructor (
> +  VOID
> +  )
> +{
> +  return EFI_SUCCESS;
> +}

(3) In the above two files, <Uefi.h> should not be included, plus the constructors should return RETURN_SUCCESS.

(4) It's a bit higher up in the patch (I'm mentioning it here only for keeping the previous points focused): if you diff the two new files against each other, you get

-  This constructor function does not have anything to do.
+  This constructor function does not have to do anything.

Please consider unifying this.

Then, we get to the INF files:

> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> index 0e74fe94cb..65d8683f1f 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> @@ -30,6 +30,7 @@
>  
>  [Sources]
>    DebugLib.c
> +  DebugLibDetect.c
>  
>  [Packages]
>    MdePkg/MdePkg.dec
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
> new file mode 100644
> index 0000000000..93763d47dd
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
> @@ -0,0 +1,52 @@
> +## @file
> +#  Instance of Debug Library for the QEMU debug console port.
> +#  It uses Print Library to produce formatted output strings.
> +#
> +#  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2017, Red Hat, Inc.<BR>
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD License
> +#  which accompanies this distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php.
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = PlatformRomDebugLibIoPort
> +  FILE_GUID                      = CEB0D9D3-328F-4C24-8C02-28FA1986AE1B
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = DebugLib
> +  CONSTRUCTOR                    = PlatformRomDebugLibIoPortConstructor
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 IPF EBC
> +#
> +
> +[Sources]
> +  DebugLib.c
> +  DebugLibDetectRom.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  IoLib
> +  PcdLib
> +  PrintLib
> +  BaseLib
> +  DebugPrintErrorLevelLib
> +
> +[Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdDebugIoPort                ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue        ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask            ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel    ## CONSUMES
> +

(5) This is totally my fault, but last night I couldn't think of it:

The LIBRARY_CLASS assignment in the [Defines] section supports a syntax that restricts the library instance to specific client module types. Such as:

  LIBRARY_CLASS                  = DebugLib|SEC
  LIBRARY_CLASS                  = DebugLib|PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION

(The module types need not be sorted "logically" within LIBRARY_CLASS, but I did that above for a pleasing read.)

Such restrictions are helpful because then an invalid / unintended library class resolution in a DSC file is caught at build time; it won't get to cause weird behavior at runtime.

Please consider adding these lists in v3.

> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index c2f534fdbf..7ccb61147f 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -207,7 +207,7 @@
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>  !else
> -  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> +  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
>  !endif
>    ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>    ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 9f300a2e6f..237ec71b5e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -212,7 +212,7 @@
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>  !else
> -  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> +  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
>  !endif
>    ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>    ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 1ffcf37f8b..a5047fa38e 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -212,7 +212,7 @@
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>  !else
> -  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> +  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
>  !endif
>    ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>    ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf
> 

This is fine, but I'll use the opportunity to ask you to tweak the git config of your edk2 clone a bit :) The below hints are from <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>.

(6) Looking at the above hunks, it is not readily apparent to the reviewer what section of the DSC file is being patched. It can be improved:

(6a) run:

git config diff.ini.xfuncname '^\[[A-Za-z0-9_., ]+]'

(6b) add the following to ".git/info/attributes":

*.dec     diff=ini
*.dsc     diff=ini
*.dsc.inc diff=ini
*.fdf     diff=ini
*.fdf.inc diff=ini
*.inf     diff=ini

As a result, the hunks will look like:

> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index c2f534fdbf3b..7ccb61147f27 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -207,7 +207,7 @@ [LibraryClasses.common.SEC]
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>  !else
> -  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> +  DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
>  !endif
>    ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf
>    ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf

Note "[LibraryClasses.common.SEC]" after the "@@" marker.

(Yes, we could carry a .gitattributes file in the tree, but that would require us to agree upon it.)

(7) Consider creating a file called "edk2.diff.order", with the following contents:

*.dec
*.dsc.inc
*.dsc
*.fdf.inc
*.fdf
*.inf
*.h
*.vfr
*.c

and setting the "diff.orderFile" config knob to the pathname of that file.

Then the reviewer will get to see hunks in the following order:
- package declaration file changes,
- platform DSC and flash description changes,
- module INF changes
- C header changes,
- VFR (visual form representation) changes,
- finally, C language changes.

(Yes, we could carry an order file in the tree as well, but my attempt to do it for QEMU had failed, so I didn't try posting it for edk2.)

I hope you aren't overly annoyed by the above wall of text. Do you have time for a v3?

Thank you!
Laszlo