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

[Laszlo Ersek <lersek@redhat.com>]

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>