Re: [PATCH 0/8] IoLib class library

["Gao, Liming" <liming.gao@intel.com>]

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
-----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