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

[Laszlo Ersek <lersek@redhat.com>]

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