[PATCH v2 0/2] OvmfPkg: save on I/O port accesses when the debug port is not in use

[PATCH v2 0/2] OvmfPkg: save on I/O port accesses when the debug port is not in use

[Paolo Bonzini]

This is version 2 of the patch I sent yesterday, now with proper SEC
support.  The series makes OvmfPkg's PlatformDebugLibIoPort library
skip I/O port writes when the debug port device wasn't added to the
virtual machine.

Patch 1 creates a separate PlatformDebugLibIoPort instance for SEC, so
that the non-SEC version will be able to use a writable global variable.
Patch 2 then adds the detection machinery to both library instances.

The commit messages in both patches liberally pillage Laszlo's v1 review.

Thanks,

Paolo

Paolo Bonzini (2):
  OvmfPkg: create a separate PlatformDebugLibIoPort instance for SEC
  OvmfPkg: save on I/O port accesses when the debug port is not in use

 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c  | 41 ++++++++--------
 .../PlatformDebugLibIoPort/DebugLibDetect.c        | 55 +++++++++++++++++++++
 .../PlatformDebugLibIoPort/DebugLibDetect.h        | 56 ++++++++++++++++++++++
 .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 48 +++++++++++++++++++
 .../PlatformDebugLibIoPort.inf                     |  1 +
 .../PlatformRomDebugLibIoPort.inf                  | 52 ++++++++++++++++++++
 OvmfPkg/OvmfPkgIa32.dsc                            |  2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                         |  2 +-
 OvmfPkg/OvmfPkgX64.dsc                             |  2 +-
 9 files changed, 237 insertions(+), 22 deletions(-)
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf

-- 
2.14.3

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

[Paolo Bonzini]

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

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.
 
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;
+}
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
+
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
-- 
2.14.3

[PATCH 2/2] OvmfPkg: save on I/O port accesses when the debug port is not in use

[Paolo Bonzini]

When SEV is enabled, every debug message printed by OVMF to the
QEMU debug port traps from the guest to QEMU character by character
because "REP OUTSB" cannot be used by IoWriteFifo8.  Furthermore,
when OVMF is built with the DEBUG_VERBOSE bit (value 0x00400000)
enabled in "gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel", then the
OvmfPkg/IoMmuDxe driver, and the OvmfPkg/Library/BaseMemEncryptSevLib
library instance that is built into it, produce a huge amount of
log messages.  Therefore, in SEV guests, the boot time impact is huge
(about 45 seconds _additional_ time spent writing to the debug port).

While these messages are very useful for analyzing guest behavior,
most of the time the user won't be capturing the OVMF debug log.
In fact libvirt does not provide a method for configuring log capture;
users that wish to do this (or are instructed to do this) have to resort
to <qemu:arg>.

The debug console device provides a handy detection mechanism; when read,
it returns 0xE9 (which is very much unlike the 0xFF that is returned by
an unused port).  Use it to skip the possibly expensive OUT instructions
when the debug I/O port isn't plugged anywhere.

For SEC, the debug port has to be read before each full message.
However:

- if the debug port is available, then reading one byte before writing
a full message isn't tragic, especially because SEC doesn't print many
messages

- if the debug port is not available, then reading one byte instead of
writing a full message is still a win.

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  | 26 ++++++++--
 .../PlatformDebugLibIoPort/DebugLibDetect.c        | 29 +++++++++--
 .../PlatformDebugLibIoPort/DebugLibDetect.h        | 56 ++++++++++++++++++++++
 .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 20 +++++++-
 4 files changed, 122 insertions(+), 9 deletions(-)
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h

diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
index a5572a8eeb..79486ac8a6 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
@@ -23,6 +23,7 @@
 #include <Library/PcdLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugPrintErrorLevelLib.h>
+#include "DebugLibDetect.h"
 
 //
 // Define the maximum debug and assert message length that this library supports
@@ -62,9 +63,9 @@ DebugPrint (
   ASSERT (Format != NULL);
 
   //
-  // Check driver debug mask value and global mask
+  // Do nothing if the global mask disables this message or the device is inactive
   //
-  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
+  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 || !PlatformDebugLibIoPortFound ()) {
     return;
   }
 
@@ -121,9 +122,11 @@ DebugAssert (
              FileName, (UINT64)LineNumber, Description);
 
   //
-  // Send the print string to the debug I/O port
+  // Send the print string to the debug I/O port, if present
   //
-  IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
+  if (PlatformDebugLibIoPortFound ()) {
+    IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
+  }
 
   //
   // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
@@ -266,3 +269,18 @@ DebugPrintLevelEnabled (
 {
   return (BOOLEAN) ((ErrorLevel & PcdGet32(PcdFixedDebugPrintErrorLevel)) != 0);
 }
+
+/**
+  Return the result of detecting the debug I/O port device.
+
+  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortDetect (
+  VOID
+  )
+{
+  return IoRead8 (PcdGet16 (PcdDebugIoPort)) == BOCHS_DEBUG_PORT_MAGIC;
+}
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
index fee908861b..610987aca9 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
@@ -1,6 +1,6 @@
 /** @file
-  Constructor code for QEMU debug port library.
-  Non-SEC instance.
+  Detection code for QEMU debug port.
+  Non-SEC instance, caches the result of detection.
 
   Copyright (c) 2017, Red Hat, Inc.<BR>
   This program and the accompanying materials
@@ -15,9 +15,16 @@
 
 #include <Base.h>
 #include <Uefi.h>
+#include "DebugLibDetect.h"
+
+//
+// Set to TRUE if the debug I/O port is enabled
+//
+STATIC BOOLEAN mDebugIoPortFound = FALSE;
 
 /**
-  This constructor function does not have anything to do.
+  This constructor function checks if the debug I/O port device is present,
+  caching the result for later use.
 
   @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
 
@@ -28,5 +35,21 @@ PlatformDebugLibIoPortConstructor (
   VOID
   )
 {
+  mDebugIoPortFound = PlatformDebugLibIoPortDetect();
   return EFI_SUCCESS;
 }
+
+/**
+  Return the cached result of detecting the debug I/O port device.
+
+  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortFound (
+  VOID
+  )
+{
+  return mDebugIoPortFound;
+}
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
new file mode 100644
index 0000000000..c34ca9c72b
--- /dev/null
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
@@ -0,0 +1,56 @@
+/** @file
+  Base Debug library instance for QEMU debug port.
+  It uses PrintLib to send debug messages to a fixed I/O port.
+
+  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.
+
+**/
+
+#ifndef __DEBUG_IO_PORT_DETECT_H__
+#define __DEBUG_IO_PORT_DETECT_H__
+
+#include <Base.h>
+#include <Uefi.h>
+
+//
+// The constant value that is read from the debug I/O port
+//
+#define BOCHS_DEBUG_PORT_MAGIC    0xE9
+
+
+/**
+  Helper function to return whether the virtual machine has a debug I/O port.
+  PlatformDebugLibIoPortFound can call this function directly or cache the
+  result.
+
+  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortDetect (
+  VOID
+  );
+
+/**
+  Return whether the virtual machine has a debug I/O port.  DebugLib.c
+  calls this function instead of PlatformDebugLibIoPortDetect, to allow
+  caching if possible.
+
+  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortFound (
+  VOID
+  );
+
+#endif
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
index 407fe613ff..f71b6567dc 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
@@ -1,6 +1,6 @@
 /** @file
-  Constructor code for QEMU debug port library.
-  SEC instance.
+  Detection code for QEMU debug port.
+  SEC instance, cannot cache the result of detection.
 
   Copyright (c) 2017, Red Hat, Inc.<BR>
   This program and the accompanying materials
@@ -15,6 +15,7 @@
 
 #include <Base.h>
 #include <Uefi.h>
+#include "DebugLibDetect.h"
 
 /**
   This constructor function does not have to do anything.
@@ -30,3 +31,18 @@ PlatformRomDebugLibIoPortConstructor (
 {
   return EFI_SUCCESS;
 }
+
+/**
+  Return the result of detecting the debug I/O port device.
+
+  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortFound (
+  VOID
+  )
+{
+  return PlatformDebugLibIoPortDetect ();
+}
-- 
2.14.3

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

[Laszlo Ersek]

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

Re: [PATCH 2/2] OvmfPkg: save on I/O port accesses when the debug port is not in use

[Laszlo Ersek]

On 11/16/17 11:47, Paolo Bonzini wrote:
> When SEV is enabled, every debug message printed by OVMF to the
> QEMU debug port traps from the guest to QEMU character by character
> because "REP OUTSB" cannot be used by IoWriteFifo8.  Furthermore,
> when OVMF is built with the DEBUG_VERBOSE bit (value 0x00400000)
> enabled in "gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel", then the
> OvmfPkg/IoMmuDxe driver, and the OvmfPkg/Library/BaseMemEncryptSevLib
> library instance that is built into it, produce a huge amount of
> log messages.  Therefore, in SEV guests, the boot time impact is huge
> (about 45 seconds _additional_ time spent writing to the debug port).
> 
> While these messages are very useful for analyzing guest behavior,
> most of the time the user won't be capturing the OVMF debug log.
> In fact libvirt does not provide a method for configuring log capture;
> users that wish to do this (or are instructed to do this) have to resort
> to <qemu:arg>.
> 
> The debug console device provides a handy detection mechanism; when read,
> it returns 0xE9 (which is very much unlike the 0xFF that is returned by
> an unused port).  Use it to skip the possibly expensive OUT instructions
> when the debug I/O port isn't plugged anywhere.
> 
> For SEC, the debug port has to be read before each full message.
> However:
> 
> - if the debug port is available, then reading one byte before writing
> a full message isn't tragic, especially because SEC doesn't print many
> messages
> 
> - if the debug port is not available, then reading one byte instead of
> writing a full message is still a win.
> 
> 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  | 26 ++++++++--
>  .../PlatformDebugLibIoPort/DebugLibDetect.c        | 29 +++++++++--
>  .../PlatformDebugLibIoPort/DebugLibDetect.h        | 56 ++++++++++++++++++++++
>  .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 20 +++++++-
>  4 files changed, 122 insertions(+), 9 deletions(-)
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
> 
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index a5572a8eeb..79486ac8a6 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -23,6 +23,7 @@
>  #include <Library/PcdLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugPrintErrorLevelLib.h>
> +#include "DebugLibDetect.h"
>  
>  //
>  // Define the maximum debug and assert message length that this library supports
> @@ -62,9 +63,9 @@ DebugPrint (
>    ASSERT (Format != NULL);
>  
>    //
> -  // Check driver debug mask value and global mask
> +  // Do nothing if the global mask disables this message or the device is inactive
>    //
> -  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
> +  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 || !PlatformDebugLibIoPortFound ()) {

(1) I feel terrible for wasting your time with this -- we prefer to keep
a line length of 80 characters:

  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 ||
      !PlatformDebugLibIoPortFound ()) {

("we prefer" is a bit of a lie -- I personally totally insist on it :) ,
while many people ignore it until I raise a stink about their overlong
lines. So I guess "we prefer" is an expected value, with large stddev.)

>      return;
>    }
>  
> @@ -121,9 +122,11 @@ DebugAssert (
>               FileName, (UINT64)LineNumber, Description);
>  
>    //
> -  // Send the print string to the debug I/O port
> +  // Send the print string to the debug I/O port, if present
>    //
> -  IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
> +  if (PlatformDebugLibIoPortFound ()) {
> +    IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
> +  }
>  
>    //
>    // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
> @@ -266,3 +269,18 @@ DebugPrintLevelEnabled (
>  {
>    return (BOOLEAN) ((ErrorLevel & PcdGet32(PcdFixedDebugPrintErrorLevel)) != 0);
>  }
> +
> +/**
> +  Return the result of detecting the debug I/O port device.
> +
> +  @retval BOOLEAN   TRUE if the debug I/O port device was detected.

(2) Another non-intuitive edk2 quirk; we have two possibilities for
retval documentation:

  @retval VALUE  description

and

  @return  just description

For example,

  @retval TRUE   if the debug I/O port device was detected
  @retval FALSE  otherwise

and

  @return  a BOOLEAN indicating whether the debug I/O port device was
           detected

(I apologize -- I know this is annoying.)

> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortDetect (
> +  VOID
> +  )
> +{
> +  return IoRead8 (PcdGet16 (PcdDebugIoPort)) == BOCHS_DEBUG_PORT_MAGIC;
> +}
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> index fee908861b..610987aca9 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> @@ -1,6 +1,6 @@
>  /** @file
> -  Constructor code for QEMU debug port library.
> -  Non-SEC instance.
> +  Detection code for QEMU debug port.
> +  Non-SEC instance, caches the result of detection.
>  
>    Copyright (c) 2017, Red Hat, Inc.<BR>
>    This program and the accompanying materials
> @@ -15,9 +15,16 @@
>  
>  #include <Base.h>
>  #include <Uefi.h>
> +#include "DebugLibDetect.h"
> +
> +//
> +// Set to TRUE if the debug I/O port is enabled
> +//
> +STATIC BOOLEAN mDebugIoPortFound = FALSE;
>  
>  /**
> -  This constructor function does not have anything to do.
> +  This constructor function checks if the debug I/O port device is present,
> +  caching the result for later use.
>  
>    @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
>  
> @@ -28,5 +35,21 @@ PlatformDebugLibIoPortConstructor (
>    VOID
>    )
>  {
> +  mDebugIoPortFound = PlatformDebugLibIoPortDetect();
>    return EFI_SUCCESS;
>  }
> +
> +/**
> +  Return the cached result of detecting the debug I/O port device.
> +
> +  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortFound (
> +  VOID
> +  )
> +{
> +  return mDebugIoPortFound;
> +}
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
> new file mode 100644
> index 0000000000..c34ca9c72b
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
> @@ -0,0 +1,56 @@
> +/** @file
> +  Base Debug library instance for QEMU debug port.
> +  It uses PrintLib to send debug messages to a fixed I/O port.
> +
> +  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.
> +
> +**/
> +
> +#ifndef __DEBUG_IO_PORT_DETECT_H__
> +#define __DEBUG_IO_PORT_DETECT_H__
> +
> +#include <Base.h>
> +#include <Uefi.h>

(3) As suggested before, please try to replace this with
<Uefi/UefiBaseType.h>.


I tested the patches (in SEV and non-SEV guests) and they work as
advertized. For v3:

Tested-by: Laszlo Ersek <lersek@redhat.com>

Thank you!
Laszlo

> +
> +//
> +// The constant value that is read from the debug I/O port
> +//
> +#define BOCHS_DEBUG_PORT_MAGIC    0xE9
> +
> +
> +/**
> +  Helper function to return whether the virtual machine has a debug I/O port.
> +  PlatformDebugLibIoPortFound can call this function directly or cache the
> +  result.
> +
> +  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortDetect (
> +  VOID
> +  );
> +
> +/**
> +  Return whether the virtual machine has a debug I/O port.  DebugLib.c
> +  calls this function instead of PlatformDebugLibIoPortDetect, to allow
> +  caching if possible.
> +
> +  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortFound (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> index 407fe613ff..f71b6567dc 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> @@ -1,6 +1,6 @@
>  /** @file
> -  Constructor code for QEMU debug port library.
> -  SEC instance.
> +  Detection code for QEMU debug port.
> +  SEC instance, cannot cache the result of detection.
>  
>    Copyright (c) 2017, Red Hat, Inc.<BR>
>    This program and the accompanying materials
> @@ -15,6 +15,7 @@
>  
>  #include <Base.h>
>  #include <Uefi.h>
> +#include "DebugLibDetect.h"
>  
>  /**
>    This constructor function does not have to do anything.
> @@ -30,3 +31,18 @@ PlatformRomDebugLibIoPortConstructor (
>  {
>    return EFI_SUCCESS;
>  }
> +
> +/**
> +  Return the result of detecting the debug I/O port device.
> +
> +  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortFound (
> +  VOID
> +  )
> +{
> +  return PlatformDebugLibIoPortDetect ();
> +}
>