Re: [PATCH 0/8] IoLib class library

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

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