From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 6D5538034B for ; Tue, 7 Mar 2017 14:18:41 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E24F63D94E; Tue, 7 Mar 2017 22:18:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-184.phx2.redhat.com [10.3.116.184]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v27MIc4U020618; Tue, 7 Mar 2017 17:18:39 -0500 To: Jordan Justen , "Gao, Liming" , Brijesh Singh , edk2-devel@ml01.01.org, "Kinney, Michael D" References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884287496.29188.5155874233993236979.stgit@brijesh-build-machine> <2c6593dd-12f5-9277-0c36-ffd2d6c2cc55@redhat.com> <148891718851.27104.2018366977522352345@jljusten-skl> Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, brijesh.sing@amd.com From: Laszlo Ersek Message-ID: <817d79b3-cbbd-4b0d-6c3c-0615b45f918a@redhat.com> Date: Tue, 7 Mar 2017 23:18:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <148891718851.27104.2018366977522352345@jljusten-skl> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 07 Mar 2017 22:18:42 +0000 (UTC) Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Mar 2017 22:18:41 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/07/17 21:06, Jordan Justen wrote: > On 2017-03-07 09:20:14, Laszlo Ersek wrote: >> On 03/07/17 00:27, Brijesh Singh wrote: >>> Imports IoLib into OvmfPkg to make the changes to support SEV guest. >> >> Ugh, this looks terrible. >> >> $ wc -l $(git ls-files MdePkg/Library/BaseIoLibIntrinsic/) >> 82 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf >> 24 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.uni >> 26 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicInternal.h >> 141 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.asm >> 137 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm >> 2356 MdePkg/Library/BaseIoLibIntrinsic/IoHighLevel.c >> 317 MdePkg/Library/BaseIoLibIntrinsic/IoLib.c >> 599 MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c >> 342 MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c >> 196 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c >> 214 MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c >> 736 MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c >> 411 MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c >> 228 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c >> 127 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.asm >> 126 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm >> 6062 total >> >> Jordan, Liming, if I recall correctly, you guys were leading the >> IoFifoLib discussion a few weeks back. At that time, I would have >> preferred to add those functions to a separate IoFifoLib class (like >> Brijesh originally suggested), but seeing the consensus on adding the >> Fifo primitives to IoLib instead, I didn't speak up. >> >> So now that the Fifo primitives have to be customized (unrolled), and >> the selection should be made dynamically (at runtime), what do you guys >> suggest for the implementation, without importing six thousand lines >> into OvmfPkg? >> >> I think this patch should be dropped, and the next patch (#5) should be >> applied straight to MdePkg. SEV detection happens via the CPUID >> instruction, and it is specified by a public industry standard, so >> adding the code to MdePkg looks appropriate to me. > > Yeah, I agree. (Not sure if Liming and Mike agree though. :) > > Additionally, it would be nice to have a spec citation for the "Public > Industry Standard" in the commit message. > >> If even the CPUID check should be omitted in the default case, then we >> should use a new FeaturePCD. > > Apparently we don't mind terribly about adding a cpuid call straight > into the normal flow of commonly used functions > (881813d7a93d9009c873515b043c41c4554779e4). :) > > I would say that I don't quite agree with that. Well, I'm neutral on it. If keeping the CPUID for SEV feature detection is okay as far as the coding style is concerned, I"m all for it. > And, further, it could > be that once per I/O operation has more of a perf impact than once per > flush. Do we know that cpuid time is so far down in the noise compared > to I/O that it won't matter? Well, for fifo ops at least, the overhead of CPUID shouldn't be large. > > One other thought is, should we consider a DxeSmm alternative .inf for > BaseIoLibIntrinsic.inf? I sort of dislike that, unless Brijesh can solve it with another INF file within the same directory, and minimal code duplication. (I.e., with most source files reused.) > In that case we could use a global variable to > help out. Maybe this could prevent the concern that might drive a new > PCD to be added? This crossed my mind (and then the CPUID would be executed only once per library constructor invocation), but I was (somewhat inexplicably?) worried that such a library instance would not be suitable for SEC code, and for PEI code running on different platforms (i.e., XIP from flash). Those restrictions don't seem to apply to OVMF though, so I guess the idea is worth exploring! Thanks Laszlo