Re: [PATCH 0/8] IoLib class library

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

Liming, et al,

Bottom line, in my estimation:
Supporting EfiCpuIoWidthFifoUint# via the CPU I/O protocol would require significant rework of the CpuIo2Dxe driver.
(Which frankly is a bit beyond of the scope of my proposed patch set)

So, if you are in agreement, I'm prepared to submit a "v2" to correct the coding-style issues.

Thanks,
Leo.

> -----Original Message-----
> From: Duran, Leo
> Sent: Wednesday, January 11, 2017 10: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