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