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

[Laszlo Ersek <lersek@redhat.com>]

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 ();
> +}
>