Re: [PATCH v3 0/4] BaseIoFifoLib

["Duran, Leo" <leo.duran@amd.com>]

OK Liming, I think I understand now.
Thanks,
Leo

> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Monday, January 09, 2017 11:34 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:
>   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