Re: [PATCH v3 0/4] BaseIoFifoLib

[Laszlo Ersek <lersek@redhat.com>]

On 01/09/17 15:22, Duran, Leo wrote:
> 
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Sunday, January 08, 2017 9:11 PM
>> To: Duran, Leo <leo.duran@amd.com>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; 'Laszlo Ersek' <lersek@redhat.com>; edk2-
>> devel@lists.01.org
>> Cc: Singh, Brijesh <brijesh.singh@amd.com>; Fan, Jeff <jeff.fan@intel.com>;
>> Kinney, Michael D <michael.d.kinney@intel.com>; Ma, Maurice
>> <maurice.ma@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>;
>> Ni, Ruiyu <ruiyu.ni@intel.com>; Steele, Kelly <kelly.steele@intel.com>; Wei,
>> David <david.wei@intel.com>; Guo, Mang <mang.guo@intel.com>
>> Subject: RE: [PATCH v3 0/4] BaseIoFifoLib
>>
>> Leo:
>>   IoLib Library class is designed from the functionality, not code
>> implementation. So, many IO operations are included in this library class. If
>> developers want to use IO API, they only need to check IoLib library class.
>> After add new APIs, we need to update all IoLib library instances to
>> implement them. And, if any library API implementation has the different
>> version, the full library instance will have to be copied to another instance. I
>> know your concern is to duplicate the library implementation. But, I think this
>> is the separate topic to optimize the library implementation and reuse the
>> same source file. Other library instances may have the same issue. So, I
>> suggest you submit bugzilla for this optimization request. We will figure out
>> the solution and review it in this mail list.
>>
>> Thanks
>> Liming
> [Duran, Leo] 
> Hi Liming,
> 
> I'm not sure I follow what you mean by an 'optimization request'.

I think under "optimization", Liming means eliminating code duplication
between IoLib library instances. That is, if I understand correctly,
Liming and Jordan suggest that you first add the new interfaces to the
IoLib class header, add (--> duplicate) the implementation to all
library instances, then file a BZ about eliminating code duplication
between the library instances.

This is my understanding anyway.

Thanks
Laszlo


> At present IoLIb does *not* include the Fifo routines that I've referred to, so I'm simply proposing to wrap the Fifo routines into in a library.
> Moreover, as you just said, I’m also proposing not using IoLib to avoid having to duplicate all of the functionality in IoLib.
> 
> Can you please give me a bit more detail as to what the 'optimization request' would be?
> (i.e., should that request read exactly as I've proposed so far, proposing the creation of an IoFifoLib?)
> I'll submit Bugzilla once I better understand what needs to be in it.
> 
> Thanks,
> Leo
> 
> 
> 
>>> -----Original Message-----
>>> From: Duran, Leo [mailto:leo.duran@amd.com]
>>> Sent: Sunday, January 08, 2017 1:17 AM
>>> To: Justen, Jordan L <jordan.l.justen@intel.com>; 'Laszlo Ersek'
>>> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; edk2-
>>> devel@lists.01.org
>>> Cc: Singh, Brijesh <brijesh.singh@amd.com>; Fan, Jeff
>>> <jeff.fan@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>> Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince
>>> <prince.agyeman@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Steele,
>>> Kelly <kelly.steele@intel.com>; Wei, David <david.wei@intel.com>; Guo,
>>> Mang <mang.guo@intel.com>
>>> Subject: RE: [PATCH v3 0/4] BaseIoFifoLib
>>>
>>> Jordan, Liming, et al,
>>>
>>> It turns out that the runtime enablement of SEV feature that I referred
>>> to can be detected in hardware; so instead of requiring 'driver' code
>>> to set a dynamic PCD, the override Fifo routines could do a runtime
>>> check like this:
>>>
>>> // In override version of the Fifo library
>>> fifo_foo()
>>> {
>>> 	If (SEV_Enabled()) {
>>> 		// don't use REP ins/outs
>>> 	} else {
>>> 		// use REP ins/outs
>>> 	}
>>> }
>>> In essence we already have a hardware-based dynamic PCD, so the idea is
>>> to leverage it.
>>>
>>> And since we're interested in overriding just the Fifo routines, it
>>> would make better sense to keep them in a separate library (as proposed in
>> the patch set).
>>> Leo.
>>>
>>>> -----Original Message-----
>>>> From: Jordan Justen [mailto:jordan.l.justen@intel.com]
>>>> Sent: Friday, January 06, 2017 6:50 PM
>>>> To: Duran, Leo <leo.duran@amd.com>; 'Laszlo Ersek'
>>>> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>;
>>>> edk2-devel@lists.01.org
>>>> Cc: Singh, Brijesh <brijesh.singh@amd.com>; Fan, Jeff
>>>> <jeff.fan@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince
>>>> <prince.agyeman@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Steele,
>>>> Kelly <kelly.steele@intel.com>; Wei, David <david.wei@intel.com>;
>>>> Guo, Mang <mang.guo@intel.com>
>>>> Subject: RE: [PATCH v3 0/4] BaseIoFifoLib
>>>>
>>>> On 2017-01-06 07:23:47, Duran, Leo wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>>>> Sent: Friday, January 06, 2017 5:12 AM
>>>>>> To: Gao, Liming <liming.gao@intel.com>; Duran, Leo
>>>>>> <leo.duran@amd.com>; edk2-devel@lists.01.org
>>>>>> <edk2-devel@ml01.01.org>
>>>>>> Cc: Singh, Brijesh <brijesh.singh@amd.com>; Justen, Jordan L
>>>>>> <jordan.l.justen@intel.com>; Fan, Jeff <jeff.fan@intel.com>;
>>>>>> Kinney, Michael D <michael.d.kinney@intel.com>; Ma, Maurice
>>>>>> <maurice.ma@intel.com>; Agyeman, Prince
>>>> <prince.agyeman@intel.com>;
>>>>>> Ni, Ruiyu <ruiyu.ni@intel.com>; Steele, Kelly
>>>>>> <kelly.steele@intel.com>; Wei, David <david.wei@intel.com>; Guo,
>>>>>> Mang <mang.guo@intel.com>
>>>>>> Subject: Re: [PATCH v3 0/4] BaseIoFifoLib
>>>>>>
>>>>>> On 01/06/17 07:02, Gao, Liming wrote:
>>>>>>> Leo:
>>>>>>> FifoIo is one width type of EFI_CPU_IO_PROTOCOL_WIDTH. So, how
>>>>>>> about add new APIs into IoLib together with other Io APIs? If
>>>>>>> so, no new library class is required. Platform DSC files are
>>>>>>> not required to be changed.
>>>>>>
>>>>>> But then all of the IoLib instances will have to be extended too:
>>>>>>
>>>>>> IntelFrameworkPkg/Library/DxeIoLibCpuIo/DxeIoLibCpuIo.inf
>>>>>> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>>>>>> MdePkg/Library/DxeIoLibCpuIo2/DxeIoLibCpuIo2.inf
>>>>>> MdePkg/Library/DxeIoLibEsal/DxeIoLibEsal.inf
>>>>>> MdePkg/Library/PeiIoLibCpuIo/PeiIoLibCpuIo.inf
>>>>>> MdePkg/Library/SmmIoLibSmmCpuIo2/SmmIoLibSmmCpuIo2.inf
>>>>>>
>>>>>> Thanks,
>>>>>> Laszlo
>>>>>>
>>>>> [Duran, Leo] Correct.
>>>>> As I mentioned, one of the reasons for the new IoFifo library is to
>>>>> be able to override it without having to duplicate the complete IoLib.
>>>>>
>>>>
>>>> I agree with Liming about adding the functions to IoLib instead.
>>>>
>>>> Perhaps a PCD could be added to control if rep i/o instructions are
>>>> used.
>>>>
>>>> -Jordan