Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import BaseIoLibIntrinsic package

[Jordan Justen <jordan.l.justen@intel.com>]

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