From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DFFB9220D4BE0 for ; Wed, 15 Nov 2017 16:39:31 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4DCDB883A2; Thu, 16 Nov 2017 00:43:40 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-60.rdu2.redhat.com [10.10.120.60]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2345760603; Thu, 16 Nov 2017 00:43:38 +0000 (UTC) To: Paolo Bonzini , edk2-devel@lists.01.org References: <20171115173052.1503-1-pbonzini@redhat.com> Cc: Ard Biesheuvel , "Jordan Justen (Intel address)" From: Laszlo Ersek Message-ID: <0db727ab-4ae1-0497-6abe-bd39b9a452e2@redhat.com> Date: Thu, 16 Nov 2017 01:43:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171115173052.1503-1-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 16 Nov 2017 00:43:40 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg: PlatformDebugLibIoPort: save on I/O port accesses when the debug port is not in use X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Nov 2017 00:39:32 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 . > > 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 > Signed-off-by: Paolo Bonzini > --- > 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 >