Re: [PATCH 0/8] IoLib class library

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

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