Re: [PATCH 0/8] IoLib class library

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

Leo:
  Current IO driver does support EfiCpuIoWidthFifoUint#, although CpuIo and PeiCpuIo driver doesn't support the optimized FifoIo operation. So, I think rework IoLib instance has no risk. 

Thanks
Liming
>-----Original Message-----
>From: Duran, Leo [mailto:leo.duran@amd.com]
>Sent: Thursday, January 12, 2017 9:51 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
>
>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