* Re: [PATCH] OvmfPkg: PlatformDebugLibIoPort: save on I/O port accesses when the debug port is not in use
2017-11-15 17:30 [PATCH] OvmfPkg: PlatformDebugLibIoPort: save on I/O port accesses when the debug port is not in use Paolo Bonzini
@ 2017-11-16 0:43 ` Laszlo Ersek
0 siblings, 0 replies; 2+ messages in thread
From: Laszlo Ersek @ 2017-11-16 0:43 UTC (permalink / raw)
To: Paolo Bonzini, edk2-devel; +Cc: Ard Biesheuvel, Jordan Justen (Intel address)
Hi Paolo,
On 11/15/17 18:30, 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.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
Thank you for the patch! Let's get the boring formalities out of the way:
- the subject line should be no longer than 74-76 characters,
- the TianoCore Contribution Agreement is now at version 1.1 :)
- please CC all maintainers for OvmfPkg from Maintainers.txt (and for
Xen-related patches, our Xen reviewers too -- probably not relevant
here). I've CC'd Ard and Jordan now.
- "static" is written "STATIC" in edk2 just because.
- not sure if Jordan wants BOCHS_DEBUG_PORT_MAGIC elsewhere (in some
header file), I'm fine with it here
- s/driver is inactive/device is inactive/ in a code comment, I guess (?)
More importantly: the detection via 0xE9 looks great; and the static
variable for caching the detection is suitable for the PEI phase and all
later phases. However, it is not suitable for the SEC phase, because SEC
runs from read-only flash. (This is also why we can't just make
PcdDebugPrintErrorLevel, i.e. the log filter mask, "dynamic" from
"fixed" -- we could never set it for, and consume it in, SEC, dynamically.)
The solution is to have two library instances, one for SEC and another
for all other firmware phases. The second is basically what you posted;
the first one will have to read the debug port in every spot where the
second one reads the variable. This won't be super fast, but SEC doesn't
print a whole lot of messages. Also,
- if the debug port is available, then reading one byte before writing a
full message isn't tragic,
- if the debug port is not available, then reading one byte instead of
writing a full message is still a win.
Steps for that:
(1) Have two INF files rather than one, called
PlatformDebugLibIoPort.inf and (say) PlatformRomDebugLibIoPort.inf
(2) Make sure their FILE_GUIDs differ (use uuidgen)
(3) update BASE_NAMEs as necessary
(4) Factor out a function for mDebugIoPortFound, to a new header file,
in the same directory. The new file should have (a) CRLF terminators,
(b) a license block at the top, (c) #include guards. The function can be
called InternalDebugIoPortFound() or some such.
(5) Add two C files, implementing InternalDebugIoPortFound()
differently. They can be called "DebugIoPort.c" and "RomDebugIoPort.c".
(6) Add the new C files to the INF files as necessary.
(7) Grep the OVMF DSC files for
"OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf", and
update those library resolutions as necessary.
I'll let others comment as well, I'm too tired for thinking clearly.
Thank you,
Laszlo
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index 5435767c1c..06d6169dc8 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -29,6 +29,16 @@
> //
> #define MAX_DEBUG_MESSAGE_LENGTH 0x100
>
> +//
> +// The constant value that is read from the debug I/O port
> +//
> +#define BOCHS_DEBUG_PORT_MAGIC 0xE9
> +
> +//
> +// Set to TRUE if the debug I/O port is enabled
> +//
> +static BOOLEAN mDebugIoPortFound = FALSE;
> +
> /**
> This constructor function does not have to do anything.
>
> @@ -41,6 +51,7 @@ PlatformDebugLibIoPortConstructor (
> VOID
> )
> {
> + mDebugIoPortFound = IoRead8 (PcdGet16 (PcdDebugIoPort)) == BOCHS_DEBUG_PORT_MAGIC;
> return EFI_SUCCESS;
> }
>
> @@ -77,9 +88,9 @@ DebugPrint (
> ASSERT (Format != NULL);
>
> //
> - // Check driver debug mask value and global mask
> + // Do nothing if the global mask disables this message or the driver is inactive
> //
> - if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
> + if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0 || !mDebugIoPortFound) {
> return;
> }
>
> @@ -138,7 +149,9 @@ DebugAssert (
> //
> // Send the print string to the debug I/O port
> //
> - IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
> + if (mDebugIoPortFound) {
> + IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
> + }
>
> //
> // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
>
^ permalink raw reply [flat|nested] 2+ messages in thread