Re: [PATCH v3 0/4] BaseIoFifoLib

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

Leo:
  I suggest to add new IoFifo APIs into IoLib library class, not add IoFifoLib class. After add IoFifo APIs into IoLib library class, we have to update all IoLib library instances to include IoFifo API implementation. For IA32 and X64 arch, your proposed implementation will be used. For other arch, normal implementation (read same IO port in one loop) can be used. And, you say you want to implement another version IoFifo APIs. If it is requested,  I suggest you add new IoLib instance. For IoFifo APIs, your provide new implementation. For other Io APIs, you can copy the implementation from BaseIoLibIntrinsic library instance, then new IoLib instance can provide full IO APIs. That's my proposal for IoFifo APIs. 
  
  Besides, I raise one generic problem. In edk2 project, every library instance must provide the full API implementation. If one library instance only overrides some API implementation, it will have to copy other API implementation. It will bring the duplicated code in the different library instance. Current six Io library instances are just a case. I think we can figure out new solution to share the same logic between the different library instance. If this solution is ready, you can easily add new IoLib library instance with the override APIs only. So, I suggest you submit new request to support the library instance inherit from another instance. 

Thanks
Liming
>-----Original Message-----
>From: Duran, Leo [mailto:leo.duran@amd.com]
>Sent: Monday, January 09, 2017 10:23 PM
>To: Gao, Liming <liming.gao@intel.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
>
>
>> -----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'.
>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