Re: [PATCH v3 0/4] BaseIoFifoLib

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

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
>-----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