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

[Laszlo Ersek <lersek@redhat.com>]

On 03/09/17 16:36, Duran, Leo wrote:
> OK, what I'm hearing is:
> - Let's leave the Fifo routines in BaseIoLib (as we currently have)
> - And let's add the SEV checks to BaseIoLib, so that OvmPkg (et al) can consume it as-is
> 
> If so, I could put a patch-set together for that... Please confirm.

Confirmed from my side.

> BTW, I'd try to minimize the need for CPUID (e.g., check it once and cache the result)

In a general purpose BASE library instance, you can't rely on memory
(writeable memory) being available. The library instance could be linked
into SEC or PEI phase modules that (generally speaking) have no
writeable global variables.

It's only an OvmfPkg specialty that PEI has writeable global variables.
(And it comes with a non-intuitive downside: in the SMM-less build of
OVMF, on S3 resume, the PEI global variables are not re-initialized to
the values that the C language would require. This is something we
always have to keep in mind.)

I don't think the CPUID checks will have a huge performance impact,
especially because (IIUC) you only have to customize the Fifo routines.
The cost of the CPUID will be amortized over all the individual IO port
accesses (--> traps) that you will perform individually anyway.

IMO the FeaturePCD check (which will be evaluated at compile time) is a
valid requirement, the CPUID result caching is not -- not until you can
measure the CPUID's impact to be "grave" under "general" workloads. Even
then, the separate DxeSmm instance can be added incrementally (as
suggested by others).

Thanks!
Laszlo

> 
> Leo
> 
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Wednesday, March 08, 2017 7:48 PM
>> To: Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
>> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.com>
>> Cc: Lendacky, Thomas <Thomas.Lendacky@amd.com>; Duran, Leo
>> <leo.duran@amd.com>; brijesh.sing@amd.com
>> Subject: RE: [RFC PATCH v1 4/5] OvmfPkg/BaseIoLibIntrinsic: import
>> BaseIoLibIntrinsic package
>>
>>
>>
>>> -----Original Message-----
>>> From: Justen, Jordan L
>>> Sent: Thursday, March 9, 2017 2:59 AM
>>> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Brijesh Singh
>>> <brijesh.ksingh@gmail.com>; Laszlo Ersek <lersek@redhat.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-08 07:41:58, Gao, Liming wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Justen, Jordan L
>>>>
>>>>> 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.
>>>>
>>>
>>> I don't think we can attempt to write a variable to memory in generic
>>> SEC/PEI code. Some flash memory treat memory writes as an I/O for
>>> programming purposes. I think we added
>>> PcdGuidedExtractHandlerTableAddress for this reason. This is why I
>>> suggested that maybe we could add a DXE/SMM .inf where we could
>> assume
>>> writeable global variables exit.
>>>
>>> -Jordan
>>
>> I get your point. So, I suggest to always check SEV in BaseIoLib. If people
>> meet with the performance issue, DXE/SMM version IoLib can be added
>> later.
>>
>