Re: [PATCH 0/8] IoLib class library

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

Ummh...

> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Wednesday, January 11, 2017 7:39 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>; Fan,
> Jeff <jeff.fan@intel.com>; lersek@redhat.com
> Subject: RE: [edk2] [PATCH 0/8] IoLib class library
> 
> Leo:
>   I suggest you rework IoLib instances PeiIoLib, DxeIoLibCpuIo,
> DxeIoLibCpuIo2 library instance first. Then, later you only need to change
> CpuIo PEIM and driver, don't need to change library instances again.
> 
> Thanks
> Liming
[Duran, Leo] 
If I rework the IoLib instances to use EfiCpuIoWidthFifoUint#, the I/O driver will *not* do the right thing, so the  expected IoLib functionality will be broken.
Basically, the I/O driver needs to properly support EfiCpuIoWidthFifoUint# types in order for the IoLib instances to work correctly using that type.

In contrast, albeit not optimal, the changes I've already made to the IoLIb instances will function correctly with the exiting I/O driver.

So it seems like the order t=f changes needs to be:
1) Update I/O driver to support EfiCpuIoWidthFifoUint# types
2) Then update IoLib instances to use EfiCpuIoWidthFifoUint# types

But perhaps my unders5tanding of the current implementation is flawed?

Leo.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Duran, Leo
> Sent: Thursday, January 12, 2017 9:35 AM
> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>; Fan,
> Jeff <jeff.fan@intel.com>; lersek@redhat.com
> Subject: Re: [edk2] [PATCH 0/8] IoLib class library
> 
> How about this instead:
> 
> 1) I submit a "v2" to take care of the code-style issue, while making all
> instances of IoLib fully functional & complaint with the complete IoLib API.
> 2) At a later time (soon, I promise), I submit a patch for CpuIo2Dxe to
> properly support FiFo types and also rework the IoLib instances to use
> EfiCpuIoWidthFifoUint#.
> 
> Is that a deal?
> Leo
> 
> > -----Original Message-----
> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > Sent: Wednesday, January 11, 2017 7:22 PM
> > To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>;
> > Fan, Jeff <jeff.fan@intel.com>; lersek@redhat.com
> > Subject: RE: [edk2] [PATCH 0/8] IoLib class library
> >
> > Leo:
> >   CpuIo2Dxe supports FifoIo operation, because original logic includes
> > FifoIo implementation. CpuIoCheckParameter() checks the parameter,
> > doesn't performance operation. And, 64bit IO operation is not
> > supported. You add three APIs for 8, 16 and 32 Io operation. So, there is no
> lose functionality.
> > Last, OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH) (Width & 0x03);
> It
> > still supports all Width.
> >
> >  Besides, I understand your work scope for IO library update. You can
> > focus on my comment 1. If you have no bandwidth, you can submit
> > bugzillar for comment 2 to update PeiCpuIo and DxeCpuIo driver to base
> > on IoLib to implement FifoIo APIs.
> >
> > Thanks
> > Liming
> > -----Original Message-----
> > From: Duran, Leo [mailto:leo.duran@amd.com]
> > Sent: Thursday, January 12, 2017 12:29 AM
> > To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>;
> > Fan, Jeff <jeff.fan@intel.com>; lersek@redhat.com
> > Subject: RE: [edk2] [PATCH 0/8] IoLib class library
> >
> > Liming,
> >
> > However, here are some issues with trying to use Fifo types via the
> > I/O
> > protocol:
> > 1) CpuIo2Dxe.c - CpuIoCheckParameter(): Count is forced to 1 for Fifo
> types:
> >   //
> >   // For FIFO type, the target address won't increase during the access,
> >   // so treat Count as 1
> >   //
> >   if (Width >= EfiCpuIoWidthFifoUint8 && Width <=
> > EfiCpuIoWidthFifoUint64) {
> >     Count = 1;
> >   }
> >
> > 2) CpuIo2Dxe.c - CpuIoCheckParameter():Fifo types are
> > eliminated/truncated:
> >   //
> >   // Check to see if Width is in the valid range for I/O Port operations
> >   //
> >   Width = (EFI_CPU_IO_PROTOCOL_WIDTH) (Width & 0x03);
> >   if (!MmioOperation && (Width == EfiCpuIoWidthUint64)) {
> >     return EFI_INVALID_PARAMETER;
> >   }
> >
> > 3) CpuIo2Dxe.c - CpuIoServiceRead()/CpuIoServiceWrite():
> > OperationWidth is only serviced for these cases: EfiCpuIoWidthUint8,
> > EfiCpuIoWidthUint16, and EfiCpuIoWidthUint32 So the Fifo types are not
> > serviced.
> >
> >
> > > -----Original Message-----
> > > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > Sent: Tuesday, January 10, 2017 10:07 PM
> > > To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>;
> > > Fan, Jeff <jeff.fan@intel.com>; lersek@redhat.com
> > > Subject: RE: [edk2] [PATCH 0/8] IoLib class library
> > >
> > > Leo:
> > > edk2\UefiCpuPkg\CpuIo2Dxe\CpuIo2Dxe.c CpuIoServiceRead() bases on
> > > IoReadFifo8() for EfiCpuIoWidthFifoUint8 width. So, IoLibCupIo2
> > > library instance IoReadFifo8() implementation can call
> > > mCpuIo->Io.Read (mCpuIo, EfiCpuIoWidthFifoUint8, Port, Count,
> > > Buffer);
> > >
> > > Thanks
> > > Liming
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Duran, Leo
> > > Sent: Wednesday, January 11, 2017 11:37 AM
> > > To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>;
> > > Fan, Jeff <jeff.fan@intel.com>; lersek@redhat.com
> > > Subject: Re: [edk2] [PATCH 0/8] IoLib class library
> > >
> > > Liming...
> > >
> > > > -----Original Message-----
> > > > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > > Sent: Tuesday, January 10, 2017 7:42 PM
> > > > To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
> > > > Cc: Singh, Brijesh <brijesh.singh@amd.com>; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; lersek@redhat.com; Fan, Jeff
> > > > <jeff.fan@intel.com>
> > > > Subject: RE: [edk2] [PATCH 0/8] IoLib class library
> > > >
> > > > Leo:
> > > >   Thanks for your update. Here is my comments.
> > > > 1) PeiIoLib, DxeIoLibCpuIo, DxeIoLibCpuIo2 library instance can
> > > > base on CPU IO service to do FifoIo operation. They don't
> > > > implement them
> > > again.
> > > > 2) IntelFrameworkModulePkg CpuIoDxe and UefiCpuPkg CpuIoPei
> driver
> > > can
> > > > be updated to base on FifoIo API for their FifoIo implementation.
> > >
> > > [Duran, Leo] I actually considered that, but the CPU I/O driver does
> > > not provide FiFiIo service in its implementation of the
> > EFI_CPU_IO2_PROTOCOL.
> > > The CPU I/O driver does use the Fifo routines internally, and it
> > > does so when that caller request IoRead/IoWrite... see the CPU I/O
> > > service
> > routines below.
> > >
> > > > 3) One coding style issue. We don't assign value to the variable
> > declaration.
> > > > UINT8 *Buffer8 = (UINT8 *)Buffer;
> > > > ==>
> > > > UINT8 *Buffer8;
> > > > Buffer8 = (UINT8 *)Buffer;
> > >
> > > [Duran, Leo] OK, I'll change that.
> > >
> > > >
> > > > Thanks
> > > > Liming
> > > [Duran, Leo]
> > >
> > > EFI_STATUS
> > > EFIAPI
> > > CpuIoServiceRead (
> > >   IN  EFI_CPU_IO2_PROTOCOL       *This,
> > >   IN  EFI_CPU_IO_PROTOCOL_WIDTH  Width,
> > >   IN  UINT64                     Address,
> > >   IN  UINTN                      Count,
> > >   OUT VOID                       *Buffer
> > >   )
> > > {
> > >   EFI_STATUS                 Status;
> > >   UINT8                      InStride;
> > >   UINT8                      OutStride;
> > >   EFI_CPU_IO_PROTOCOL_WIDTH  OperationWidth;
> > >   UINT8                      *Uint8Buffer;
> > >
> > >   Status = CpuIoCheckParameter (FALSE, Width, Address, Count, Buffer);
> > >   if (EFI_ERROR (Status)) {
> > >     return Status;
> > >   }
> > >
> > >   //
> > >   // Select loop based on the width of the transfer
> > >   //
> > >   InStride = mInStride[Width];
> > >   OutStride = mOutStride[Width];
> > >   OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH) (Width & 0x03);
> > >
> > > #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
> > >   if (InStride == 0) {
> > >     switch (OperationWidth) {
> > >     case EfiCpuIoWidthUint8:
> > >       IoReadFifo8 ((UINTN)Address, Count, Buffer);
> > >       return EFI_SUCCESS;
> > >     case EfiCpuIoWidthUint16:
> > >       IoReadFifo16 ((UINTN)Address, Count, Buffer);
> > >       return EFI_SUCCESS;
> > >     case EfiCpuIoWidthUint32:
> > >       IoReadFifo32 ((UINTN)Address, Count, Buffer);
> > >       return EFI_SUCCESS;
> > >     default:
> > >       //
> > >       // The CpuIoCheckParameter call above will ensure that this
> > >       // path is not taken.
> > >       //
> > >       ASSERT (FALSE);
> > >       break;
> > >     }
> > >   }
> > > #endif
> > >
> > >   for (Uint8Buffer = Buffer; Count > 0; Address += InStride,
> > > Uint8Buffer += OutStride, Count--) {
> > >     if (OperationWidth == EfiCpuIoWidthUint8) {
> > >       *Uint8Buffer = IoRead8 ((UINTN)Address);
> > >     } else if (OperationWidth == EfiCpuIoWidthUint16) {
> > >       *((UINT16 *)Uint8Buffer) = IoRead16 ((UINTN)Address);
> > >     } else if (OperationWidth == EfiCpuIoWidthUint32) {
> > >       *((UINT32 *)Uint8Buffer) = IoRead32 ((UINTN)Address);
> > >     }
> > >   }
> > >
> > >   return EFI_SUCCESS;
> > > }
> > >
> > > EFI_STATUS
> > > EFIAPI
> > > CpuIoServiceWrite (
> > >   IN EFI_CPU_IO2_PROTOCOL       *This,
> > >   IN EFI_CPU_IO_PROTOCOL_WIDTH  Width,
> > >   IN UINT64                     Address,
> > >   IN UINTN                      Count,
> > >   IN VOID                       *Buffer
> > >   )
> > > {
> > >   EFI_STATUS                 Status;
> > >   UINT8                      InStride;
> > >   UINT8                      OutStride;
> > >   EFI_CPU_IO_PROTOCOL_WIDTH  OperationWidth;
> > >   UINT8                      *Uint8Buffer;
> > >
> > >   //
> > >   // Make sure the parameters are valid
> > >   //
> > >   Status = CpuIoCheckParameter (FALSE, Width, Address, Count, Buffer);
> > >   if (EFI_ERROR (Status)) {
> > >     return Status;
> > >   }
> > >
> > >   //
> > >   // Select loop based on the width of the transfer
> > >   //
> > >   InStride = mInStride[Width];
> > >   OutStride = mOutStride[Width];
> > >   OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH) (Width & 0x03);
> > >
> > > #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
> > >   if (InStride == 0) {
> > >     switch (OperationWidth) {
> > >     case EfiCpuIoWidthUint8:
> > >       IoWriteFifo8 ((UINTN)Address, Count, Buffer);
> > >       return EFI_SUCCESS;
> > >     case EfiCpuIoWidthUint16:
> > >       IoWriteFifo16 ((UINTN)Address, Count, Buffer);
> > >       return EFI_SUCCESS;
> > >     case EfiCpuIoWidthUint32:
> > >       IoWriteFifo32 ((UINTN)Address, Count, Buffer);
> > >       return EFI_SUCCESS;
> > >     default:
> > >       //
> > >       // The CpuIoCheckParameter call above will ensure that this
> > >       // path is not taken.
> > >       //
> > >       ASSERT (FALSE);
> > >       break;
> > >     }
> > >   }
> > > #endif
> > >
> > >   for (Uint8Buffer = (UINT8 *)Buffer; Count > 0; Address +=
> > > InStride, Uint8Buffer += OutStride, Count--) {
> > >     if (OperationWidth == EfiCpuIoWidthUint8) {
> > >       IoWrite8 ((UINTN)Address, *Uint8Buffer);
> > >     } else if (OperationWidth == EfiCpuIoWidthUint16) {
> > >       IoWrite16 ((UINTN)Address, *((UINT16 *)Uint8Buffer));
> > >     } else if (OperationWidth == EfiCpuIoWidthUint32) {
> > >       IoWrite32 ((UINTN)Address, *((UINT32 *)Uint8Buffer));
> > >     }
> > >   }
> > >
> > >   return EFI_SUCCESS;
> > > }
> > > > -----Original Message-----
> > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > > > Behalf Of Leo Duran
> > > > Sent: Wednesday, January 11, 2017 7:56 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: brijesh.singh@amd.com; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>;
> > > > Leo Duran <leo.duran@amd.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; lersek@redhat.com; Fan, Jeff
> > > > <jeff.fan@intel.com>
> > > > Subject: [edk2] [PATCH 0/8] IoLib class library
> > > >
> > > > The UefiCpuPkg/CpuIo2Dxe driver and the QemuCfgLib library have
> > > > duplicate implementations of I/O Fifo routines. The patch series
> > > > moves the I/O Fifo routines into the BaseIoLibIntrinsic library
> > > > and expands the IoLib class to include the ported I/O Fifo routines.
> > > >
> > > > The Fifo routines moved from the UefiCpuPkg/CpuIo2Dxe driver
> > > > support
> > > > IA32 and X64 natively, and other architectures are supported by
> > > > simply looping through the basic IoRead/IoWrite routines as
> appropiate.
> > > >
> > > > The intent of this patch series is twofold:
> > > > 1) Integrate I/O Fifo routines into the IoLib class library.
> > > > 2) Allow override of IoLib as may be required to support specific
> > > > hardware implementations, such as AMD's Secure Encrypted
> > > > Virtualization
> > > (SEV).
> > > >
> > > > Leo Duran (8):
> > > >   MdePkg: Expand BaseIoLibIntrinsic (IoLib class) library
> > > >   MdePkg/DxeIoLibCpuIo2: Add new Fifo routines in IoLib class
> > > >   MdePkg/DxeIoLibEsal: Add new Fifo routines in IoLib class
> > > >   MdePkg/PeiIoLibCpuIo: Add new Fifo routines in IoLib class
> > > >   MdePkg/SmmIoLibSmmCpuIo2: Add new Fifo routines in IoLib class
> > > >   IntelFrameworkPkg/DxeIoLibCpuIo: Add new Fifo routines in IoLib
> class
> > > >   UefiCpuPkg: Modify CpuIo2Dxe to use new IoLib class library
> > > >   OvmfPkg: Modify QemuFwCfgLib to use new IoLib class library
> > > >
> > > >  IntelFrameworkPkg/Library/DxeIoLibCpuIo/IoLib.c    | 203
> > > > +++++++++++++++++++++
> > > >  MdePkg/Include/Library/IoLib.h                     | 158 ++++++++++++++++
> > > >  .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf      |   6 +-
> > > >  .../Library/BaseIoLibIntrinsic}/Ia32/IoFifo.asm    |   1 +
> > > >  .../Library/BaseIoLibIntrinsic}/Ia32/IoFifo.nasm   |   1 +
> > > >  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c       | 182
> > > > ++++++++++++++++++
> > > >  MdePkg/Library/BaseIoLibIntrinsic/IoLibEbc.c       | 179
> > > > ++++++++++++++++++
> > > >  MdePkg/Library/BaseIoLibIntrinsic/IoLibIpf.c       | 201
> > > > ++++++++++++++++++++
> > > >  .../Library/BaseIoLibIntrinsic}/X64/IoFifo.asm     |   1 +
> > > >  .../Library/BaseIoLibIntrinsic}/X64/IoFifo.nasm    |   1 +
> > > >  MdePkg/Library/DxeIoLibCpuIo2/IoLib.c              | 203
> > > > +++++++++++++++++++++
> > > >  MdePkg/Library/DxeIoLibEsal/IoLib.c                | 203
> > > > +++++++++++++++++++++
> > > >  MdePkg/Library/PeiIoLibCpuIo/IoLib.c               | 203
> > > > +++++++++++++++++++++
> > > >  MdePkg/Library/SmmIoLibSmmCpuIo2/IoLib.c           | 203
> > > > +++++++++++++++++++++
> > > >  OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm  |  55 ------
> > > >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c        |  54 +-----
> > > >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf      |   7 +-
> > > >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf   |   7 +-
> > > >  OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm   |  52 ------
> > > >  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c                   |   3 +-
> > > >  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf                 |  11 +-
> > > >  UefiCpuPkg/CpuIo2Dxe/IoFifo.h                      | 176 ------------------
> > > >  22 files changed, 1751 insertions(+), 359 deletions(-)  rename
> > > > {UefiCpuPkg/CpuIo2Dxe =>
> > > > MdePkg/Library/BaseIoLibIntrinsic}/Ia32/IoFifo.asm (94%)  rename
> > > > {UefiCpuPkg/CpuIo2Dxe =>
> > > > MdePkg/Library/BaseIoLibIntrinsic}/Ia32/IoFifo.nasm (94%)  rename
> > > > {UefiCpuPkg/CpuIo2Dxe =>
> > > > MdePkg/Library/BaseIoLibIntrinsic}/X64/IoFifo.asm (95%)  rename
> > > > {UefiCpuPkg/CpuIo2Dxe =>
> > > > MdePkg/Library/BaseIoLibIntrinsic}/X64/IoFifo.nasm (95%)  delete
> > > > mode
> > > > 100644 OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm
> > > >  delete mode 100644
> > > > OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm
> > > >  delete mode 100644 UefiCpuPkg/CpuIo2Dxe/IoFifo.h
> > > >
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > edk2-devel mailing list
> > > > edk2-devel@lists.01.org
> > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel