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

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

[Paolo Bonzini]

This is version 3 of the series to skip debug port I/O port writes
when the debug port device wasn't added to the virtual machine.
The differences from v2 are entirely cosmetic, and I'm including them
at the end of this message for ease of review.

Thanks,

Paolo

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

 OvmfPkg/OvmfPkgIa32.dsc                                              |  2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                                           |  2 +-
 OvmfPkg/OvmfPkgX64.dsc                                               |  2 +-
 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf    |  3 ++-
 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 52 +++++++++++++++++++++++++++++++++++++++++
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h              | 55 ++++++++++++++++++++++++++++++++++++++++++++
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c                    | 44 +++++++++++++++++++----------------
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c              | 55 ++++++++++++++++++++++++++++++++++++++++++++
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c           | 48 ++++++++++++++++++++++++++++++++++++++
 9 files changed, 241 insertions(+), 24 deletions(-)
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c

-- 
2.14.3

diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
index 65d8683f1f..de3c2f542b 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
@@ -21,7 +21,7 @@ [Defines]
   FILE_GUID                      = DF934DA3-CD31-49FE-AF50-B3C87C79325F
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = DebugLib
+  LIBRARY_CLASS                  = DebugLib|PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION
   CONSTRUCTOR                    = PlatformDebugLibIoPortConstructor
 
 #
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
index 93763d47dd..491c0318de 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
@@ -21,7 +21,7 @@ [Defines]
   FILE_GUID                      = CEB0D9D3-328F-4C24-8C02-28FA1986AE1B
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = DebugLib
+  LIBRARY_CLASS                  = DebugLib|SEC
   CONSTRUCTOR                    = PlatformRomDebugLibIoPortConstructor
 
 #
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
index c34ca9c72b..1f739b55d8 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
@@ -17,7 +17,6 @@
 #define __DEBUG_IO_PORT_DETECT_H__
 
 #include <Base.h>
-#include <Uefi.h>
 
 //
 // The constant value that is read from the debug I/O port
@@ -30,7 +29,8 @@
   PlatformDebugLibIoPortFound can call this function directly or cache the
   result.
 
-  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
 
 **/
 BOOLEAN
@@ -44,7 +44,8 @@ PlatformDebugLibIoPortDetect (
   calls this function instead of PlatformDebugLibIoPortDetect, to allow
   caching if possible.
 
-  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
 
 **/
 BOOLEAN
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
index 79486ac8a6..36cde54976 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
@@ -15,7 +15,6 @@
 **/
 
 #include <Base.h>
-#include <Uefi.h>
 #include <Library/DebugLib.h>
 #include <Library/BaseLib.h>
 #include <Library/IoLib.h>
@@ -63,9 +62,10 @@ DebugPrint (
   ASSERT (Format != NULL);
 
   //
-  // Do nothing if the global mask disables this message or the device is inactive
+  // Check if the global mask disables this message or the device is inactive
   //
-  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 || !PlatformDebugLibIoPortFound ()) {
+  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 ||
+      !PlatformDebugLibIoPortFound ()) {
     return;
   }
 
@@ -273,7 +273,8 @@ DebugPrintLevelEnabled (
 /**
   Return the result of detecting the debug I/O port device.
 
-  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
 
 **/
 BOOLEAN
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
index 610987aca9..81c44eece9 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
@@ -14,7 +14,6 @@
 **/
 
 #include <Base.h>
-#include <Uefi.h>
 #include "DebugLibDetect.h"
 
 //
@@ -26,7 +25,7 @@ STATIC BOOLEAN mDebugIoPortFound = FALSE;
   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.
+  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
 
 **/
 RETURN_STATUS
@@ -36,13 +35,14 @@ PlatformDebugLibIoPortConstructor (
   )
 {
   mDebugIoPortFound = PlatformDebugLibIoPortDetect();
-  return EFI_SUCCESS;
+  return RETURN_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.
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
 
 **/
 BOOLEAN
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
index f71b6567dc..b950919675 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
@@ -14,13 +14,12 @@
 **/
 
 #include <Base.h>
-#include <Uefi.h>
 #include "DebugLibDetect.h"
 
 /**
-  This constructor function does not have to do anything.
+  This constructor function does not have anything to do.
 
-  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
+  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
 
 **/
 RETURN_STATUS
@@ -29,13 +28,14 @@ PlatformRomDebugLibIoPortConstructor (
   VOID
   )
 {
-  return EFI_SUCCESS;
+  return RETURN_SUCCESS;
 }
 
 /**
   Return the result of detecting the debug I/O port device.
 
-  @retval BOOLEAN   TRUE if the debug I/O port device was detected.
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
 
 **/
 BOOLEAN

[PATCH v3 1/3] OvmfPkg: make PlatformDebugLibIoPort a proper BASE library

[Paolo Bonzini]

Remove Uefi.h, which includes UefiSpec.h, and change the
return value to match the RETURN_STATUS type.

Contributed-under: TianoCore Contribution Agreement 1.1
Tested-by: Laszlo Ersek <lersek@redhat.com>
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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
index 5435767c1c..74f4d9c2d6 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
@@ -15,7 +15,6 @@
 **/
 
 #include <Base.h>
-#include <Uefi.h>
 #include <Library/DebugLib.h>
 #include <Library/BaseLib.h>
 #include <Library/IoLib.h>
@@ -32,7 +31,7 @@
 /**
   This constructor function does not have to do anything.
 
-  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
+  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
 
 **/
 RETURN_STATUS
@@ -41,7 +40,7 @@ PlatformDebugLibIoPortConstructor (
   VOID
   )
 {
-  return EFI_SUCCESS;
+  return RETURN_SUCCESS;
 }
 
 /**
-- 
2.14.3

[PATCH v3 2/3] 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
Tested-by: Laszlo Ersek <lersek@redhat.com>
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/OvmfPkgIa32.dsc                                              |  2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                                           |  2 +-
 OvmfPkg/OvmfPkgX64.dsc                                               |  2 +-
 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf    |  3 ++-
 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 52 ++++++++++++++++++++++++++++++++++++++++++++
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c                    | 15 -------------
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c              | 31 ++++++++++++++++++++++++++
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c           | 31 ++++++++++++++++++++++++++
 8 files changed, 119 insertions(+), 19 deletions(-)

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 @@ [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
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 @@ [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
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 @@ [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
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
index 0e74fe94cb..de3c2f542b 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
@@ -21,7 +21,7 @@ [Defines]
   FILE_GUID                      = DF934DA3-CD31-49FE-AF50-B3C87C79325F
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = DebugLib
+  LIBRARY_CLASS                  = DebugLib|PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION
   CONSTRUCTOR                    = PlatformDebugLibIoPortConstructor
 
 #
@@ -30,6 +30,7 @@ [Defines]
 
 [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..491c0318de
--- /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|SEC
+  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/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
index 74f4d9c2d6..5a1c86f2c3 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
@@ -28,21 +28,6 @@
 //
 #define MAX_DEBUG_MESSAGE_LENGTH  0x100
 
-/**
-  This constructor function does not have to do anything.
-
-  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
-
-**/
-RETURN_STATUS
-EFIAPI
-PlatformDebugLibIoPortConstructor (
-  VOID
-  )
-{
-  return RETURN_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..bad054f286
--- /dev/null
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
@@ -0,0 +1,31 @@
+/** @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>
+
+/**
+  This constructor function does not have anything to do.
+
+  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
+
+**/
+RETURN_STATUS
+EFIAPI
+PlatformDebugLibIoPortConstructor (
+  VOID
+  )
+{
+  return RETURN_SUCCESS;
+}
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
new file mode 100644
index 0000000000..83a118a0f7
--- /dev/null
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
@@ -0,0 +1,31 @@
+/** @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>
+
+/**
+  This constructor function does not have anything to do.
+
+  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
+
+**/
+RETURN_STATUS
+EFIAPI
+PlatformRomDebugLibIoPortConstructor (
+  VOID
+  )
+{
+  return RETURN_SUCCESS;
+}
-- 
2.14.3

[PATCH v3 3/3] 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.0
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>
---
 .../PlatformDebugLibIoPort/DebugLibDetect.h        | 57 ++++++++++++++++++++++
 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c  | 28 +++++++++--
 .../PlatformDebugLibIoPort/DebugLibDetect.c        | 30 ++++++++++--
 .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 21 +++++++-
 4 files changed, 127 insertions(+), 9 deletions(-)
 create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h

diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
new file mode 100644
index 0000000000..1f739b55d8
--- /dev/null
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
@@ -0,0 +1,57 @@
+/** @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>
+
+//
+// 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 TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
+
+**/
+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 TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortFound (
+  VOID
+  );
+
+#endif
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
index 5a1c86f2c3..36cde54976 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
@@ -22,6 +22,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
@@ -61,9 +62,10 @@ DebugPrint (
   ASSERT (Format != NULL);
 
   //
-  // Check driver debug mask value and global mask
+  // Check if the global mask disables this message or the device is inactive
   //
-  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
+  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 ||
+      !PlatformDebugLibIoPortFound ()) {
     return;
   }
 
@@ -120,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
@@ -265,3 +269,19 @@ DebugPrintLevelEnabled (
 {
   return (BOOLEAN) ((ErrorLevel & PcdGet32(PcdFixedDebugPrintErrorLevel)) != 0);
 }
+
+/**
+  Return the result of detecting the debug I/O port device.
+
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
+
+**/
+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 bad054f286..81c44eece9 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
@@ -14,9 +14,16 @@
 **/
 
 #include <Base.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 RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
 
@@ -27,5 +34,22 @@ PlatformDebugLibIoPortConstructor (
   VOID
   )
 {
+  mDebugIoPortFound = PlatformDebugLibIoPortDetect();
   return RETURN_SUCCESS;
 }
+
+/**
+  Return the cached result of detecting the debug I/O port device.
+
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortFound (
+  VOID
+  )
+{
+  return mDebugIoPortFound;
+}
diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
index 83a118a0f7..b950919675 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
@@ -14,6 +14,7 @@
 **/
 
 #include <Base.h>
+#include "DebugLibDetect.h"
 
 /**
   This constructor function does not have anything to do.
@@ -29,3 +30,19 @@ PlatformRomDebugLibIoPortConstructor (
 {
   return RETURN_SUCCESS;
 }
+
+/**
+  Return the result of detecting the debug I/O port device.
+
+  @retval TRUE   if the debug I/O port device was detected.
+  @retval FALSE  otherwise
+
+**/
+BOOLEAN
+EFIAPI
+PlatformDebugLibIoPortFound (
+  VOID
+  )
+{
+  return PlatformDebugLibIoPortDetect ();
+}
-- 
2.14.3

Re: [PATCH v3 1/3] OvmfPkg: make PlatformDebugLibIoPort a proper BASE library

[Laszlo Ersek]

On 11/16/17 21:30, Paolo Bonzini wrote:
> Remove Uefi.h, which includes UefiSpec.h, and change the
> return value to match the RETURN_STATUS type.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index 5435767c1c..74f4d9c2d6 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -15,7 +15,6 @@
>  **/
>  
>  #include <Base.h>
> -#include <Uefi.h>
>  #include <Library/DebugLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/IoLib.h>
> @@ -32,7 +31,7 @@
>  /**
>    This constructor function does not have to do anything.
>  
> -  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
> +  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
>  
>  **/
>  RETURN_STATUS
> @@ -41,7 +40,7 @@ PlatformDebugLibIoPortConstructor (
>    VOID
>    )
>  {
> -  return EFI_SUCCESS;
> +  return RETURN_SUCCESS;
>  }
>  
>  /**
> 

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

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

[Laszlo Ersek]

On 11/16/17 21:30, 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
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 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/OvmfPkgIa32.dsc                                              |  2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                                           |  2 +-
>  OvmfPkg/OvmfPkgX64.dsc                                               |  2 +-
>  OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf    |  3 ++-
>  OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 52 ++++++++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c                    | 15 -------------
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c              | 31 ++++++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c           | 31 ++++++++++++++++++++++++++
>  8 files changed, 119 insertions(+), 19 deletions(-)
> 
> 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 @@ [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
> 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 @@ [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
> 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 @@ [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
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> index 0e74fe94cb..de3c2f542b 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> @@ -21,7 +21,7 @@ [Defines]
>    FILE_GUID                      = DF934DA3-CD31-49FE-AF50-B3C87C79325F
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = DebugLib
> +  LIBRARY_CLASS                  = DebugLib|PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION
>    CONSTRUCTOR                    = PlatformDebugLibIoPortConstructor
>  
>  #
> @@ -30,6 +30,7 @@ [Defines]
>  
>  [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..491c0318de
> --- /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|SEC
> +  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/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index 74f4d9c2d6..5a1c86f2c3 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -28,21 +28,6 @@
>  //
>  #define MAX_DEBUG_MESSAGE_LENGTH  0x100
>  
> -/**
> -  This constructor function does not have to do anything.
> -
> -  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
> -
> -**/
> -RETURN_STATUS
> -EFIAPI
> -PlatformDebugLibIoPortConstructor (
> -  VOID
> -  )
> -{
> -  return RETURN_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..bad054f286
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
> @@ -0,0 +1,31 @@
> +/** @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>
> +
> +/**
> +  This constructor function does not have anything to do.
> +
> +  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PlatformDebugLibIoPortConstructor (
> +  VOID
> +  )
> +{
> +  return RETURN_SUCCESS;
> +}
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> new file mode 100644
> index 0000000000..83a118a0f7
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> @@ -0,0 +1,31 @@
> +/** @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>
> +
> +/**
> +  This constructor function does not have anything to do.
> +
> +  @retval RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PlatformRomDebugLibIoPortConstructor (
> +  VOID
> +  )
> +{
> +  return RETURN_SUCCESS;
> +}
> 

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

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

[Laszlo Ersek]

On 11/16/17 21:31, 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.0
> 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>
> ---
>  .../PlatformDebugLibIoPort/DebugLibDetect.h        | 57 ++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c  | 28 +++++++++--
>  .../PlatformDebugLibIoPort/DebugLibDetect.c        | 30 ++++++++++--
>  .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 21 +++++++-
>  4 files changed, 127 insertions(+), 9 deletions(-)
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
> 
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
> new file mode 100644
> index 0000000000..1f739b55d8
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
> @@ -0,0 +1,57 @@
> +/** @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>
> +
> +//
> +// 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 TRUE   if the debug I/O port device was detected.
> +  @retval FALSE  otherwise
> +
> +**/
> +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 TRUE   if the debug I/O port device was detected.
> +  @retval FALSE  otherwise
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortFound (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index 5a1c86f2c3..36cde54976 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -22,6 +22,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
> @@ -61,9 +62,10 @@ DebugPrint (
>    ASSERT (Format != NULL);
>  
>    //
> -  // Check driver debug mask value and global mask
> +  // Check if the global mask disables this message or the device is inactive
>    //
> -  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
> +  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 ||
> +      !PlatformDebugLibIoPortFound ()) {
>      return;
>    }
>  
> @@ -120,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
> @@ -265,3 +269,19 @@ DebugPrintLevelEnabled (
>  {
>    return (BOOLEAN) ((ErrorLevel & PcdGet32(PcdFixedDebugPrintErrorLevel)) != 0);
>  }
> +
> +/**
> +  Return the result of detecting the debug I/O port device.
> +
> +  @retval TRUE   if the debug I/O port device was detected.
> +  @retval FALSE  otherwise
> +
> +**/
> +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 bad054f286..81c44eece9 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
> @@ -14,9 +14,16 @@
>  **/
>  
>  #include <Base.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 RETURN_SUCCESS   The constructor always returns RETURN_SUCCESS.
>  
> @@ -27,5 +34,22 @@ PlatformDebugLibIoPortConstructor (
>    VOID
>    )
>  {
> +  mDebugIoPortFound = PlatformDebugLibIoPortDetect();
>    return RETURN_SUCCESS;
>  }
> +
> +/**
> +  Return the cached result of detecting the debug I/O port device.
> +
> +  @retval TRUE   if the debug I/O port device was detected.
> +  @retval FALSE  otherwise
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortFound (
> +  VOID
> +  )
> +{
> +  return mDebugIoPortFound;
> +}
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
> index 83a118a0f7..b950919675 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
> @@ -14,6 +14,7 @@
>  **/
>  
>  #include <Base.h>
> +#include "DebugLibDetect.h"
>  
>  /**
>    This constructor function does not have anything to do.
> @@ -29,3 +30,19 @@ PlatformRomDebugLibIoPortConstructor (
>  {
>    return RETURN_SUCCESS;
>  }
> +
> +/**
> +  Return the result of detecting the debug I/O port device.
> +
> +  @retval TRUE   if the debug I/O port device was detected.
> +  @retval FALSE  otherwise
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +PlatformDebugLibIoPortFound (
> +  VOID
> +  )
> +{
> +  return PlatformDebugLibIoPortDetect ();
> +}
> 

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

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

[Laszlo Ersek]

On 11/16/17 21:31, 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.0
> 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>
> ---
>  .../PlatformDebugLibIoPort/DebugLibDetect.h        | 57 ++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c  | 28 +++++++++--
>  .../PlatformDebugLibIoPort/DebugLibDetect.c        | 30 ++++++++++--
>  .../PlatformDebugLibIoPort/DebugLibDetectRom.c     | 21 +++++++-
>  4 files changed, 127 insertions(+), 9 deletions(-)
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h

(looks like you didn't pick up my T-b from the v2 review for this patch;
I'm adding that back in)

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

[Laszlo Ersek]

On 11/16/17 21:30, Paolo Bonzini wrote:
> This is version 3 of the series to skip debug port I/O port writes
> when the debug port device wasn't added to the virtual machine.
> The differences from v2 are entirely cosmetic, and I'm including them
> at the end of this message for ease of review.

That's appreciated :)

Pushed as commit range d41fd8e839a3..c09d9571300a.

Many thanks for the contribution!
Laszlo

> 
> Thanks,
> 
> Paolo
> 
> Paolo Bonzini (3):
>   OvmfPkg: make PlatformDebugLibIoPort a proper BASE library
>   OvmfPkg: create a separate PlatformDebugLibIoPort instance for SEC
>   OvmfPkg: save on I/O port accesses when the debug port is not in use
> 
>  OvmfPkg/OvmfPkgIa32.dsc                                              |  2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                                           |  2 +-
>  OvmfPkg/OvmfPkgX64.dsc                                               |  2 +-
>  OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf    |  3 ++-
>  OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf | 52 +++++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h              | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c                    | 44 +++++++++++++++++++----------------
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c              | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c           | 48 ++++++++++++++++++++++++++++++++++++++
>  9 files changed, 241 insertions(+), 24 deletions(-)
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.h
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c
>  create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c
>