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 <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Jordan Justen (Intel address) <jordan.l.jus...@intel.com>
> Signed-off-by: Paolo Bonzini <pbonz...@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 <ler...@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 ();
> +}
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
