From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <jordan.l.justen@intel.com> Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 9D10480328 for <edk2-devel@lists.01.org>; Tue, 7 Mar 2017 12:06:29 -0800 (PST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 07 Mar 2017 12:06:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,258,1486454400"; d="scan'208";a="233402129" Received: from skovesdy-mobl.amr.corp.intel.com (HELO localhost) ([10.254.188.218]) by fmsmga004.fm.intel.com with ESMTP; 07 Mar 2017 12:06:28 -0800 MIME-Version: 1.0 To: "Gao, Liming" <liming.gao@intel.com>, Brijesh Singh <brijesh.ksingh@gmail.com>, Laszlo Ersek <lersek@redhat.com>, edk2-devel@lists.01.org, "Kinney, Michael D" <michael.d.kinney@intel.com>, Message-ID: <148891718851.27104.2018366977522352345@jljusten-skl> From: Jordan Justen <jordan.l.justen@intel.com> In-Reply-To: <2c6593dd-12f5-9277-0c36-ffd2d6c2cc55@redhat.com> Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, brijesh.sing@amd.com References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884287496.29188.5155874233993236979.stgit@brijesh-build-machine> <2c6593dd-12f5-9277-0c36-ffd2d6c2cc55@redhat.com> User-Agent: alot/0.5.1 Date: Tue, 07 Mar 2017 12:06:28 -0800 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 <edk2-devel.lists.01.org> List-Unsubscribe: <https://lists.01.org/mailman/options/edk2-devel>, <mailto:edk2-devel-request@lists.01.org?subject=unsubscribe> List-Archive: <http://lists.01.org/pipermail/edk2-devel/> List-Post: <mailto:edk2-devel@lists.01.org> List-Help: <mailto:edk2-devel-request@lists.01.org?subject=help> List-Subscribe: <https://lists.01.org/mailman/listinfo/edk2-devel>, <mailto:edk2-devel-request@lists.01.org?subject=subscribe> X-List-Received-Date: Tue, 07 Mar 2017 20:06:29 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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. 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? One other thought is, should we consider a DxeSmm alternative .inf for BaseIoLibIntrinsic.inf? 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? -Jordan