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

["Gao, Liming" <liming.gao@intel.com>]

>-----Original Message-----
>From: Justen, Jordan L
>Sent: Wednesday, March 08, 2017 4:06 AM
>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>
>Cc: Thomas.Lendacky@amd.com; leo.duran@amd.com;
>brijesh.sing@amd.com
>Subject: Re: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>BaseIoLibIntrinsic package
>
>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. :)
I agree. If SEV is the public industry standard, it can be added into MdePkg Library implementation. I suggest to add spec refer in file header. 

>
>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?
>
I worry about functionality. Dose CheckSevFeature() work on Intel X86 CPU? If Intel X86 CPU doesn't support SEV, will CheckSevFeature() return 0? If Intel X86 CPU doesn't work, we need to add new PCD to control its logic. 

>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
Current patch has stored the check state into data section. In PEI phase, the data section can't be written. So, every call will check CpuId. In DXE and SMM phase, the data section can be written. The first call will cache the check state. So, no DxeSmm INF is required. 


Thanks
Liming