Re: [PATCH v3 4/4] Modify QemuFwCfgLib to use new BaseIoFifoLib library.

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

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, January 06, 2017 5:36 AM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: ruiyu.ni@intel.com; Singh, Brijesh <brijesh.singh@amd.com>;
> jordan.l.justen@intel.com; liming.gao@intel.com;
> michael.d.kinney@intel.com; jeff.fan@intel.com;
> prince.agyeman@intel.com; david.wei@intel.com
> Subject: Re: [edk2] [PATCH v3 4/4] Modify QemuFwCfgLib to use new
> BaseIoFifoLib library.
> 
> On 01/05/17 22:49, Leo Duran wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Leo Duran  <leo.duran@amd.com>
> > ---
> >  OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm | 55 ----------------
> -------
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c       | 54 +-----------------
> ----
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf     |  8 +---
> >  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf  |  8 +---
> > OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm  | 52
> > ---------------------
> >  5 files changed, 5 insertions(+), 172 deletions(-)  delete mode
> > 100644 OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm
> >  delete mode 100644
> OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm
> 
> Differences vs. v1:
> - git metadata & commit msg tags fixed, good
> - copyright notices added, okay
> - now unnecessary prototypes removed, good
> - QemuFwCfgSecLib.inf now handled (I didn't notice it in v1, sorry!), OK
> 
> However, the #include <Library/IoFifoLib.h> directive is also gone, from
> "QemuFwCfgLib.c". 
[Duran, Leo] OK, I'll fix that on the next revision. Thanks.

> I think you may have moved that directive to
> "QemuFwCfgLibInternal.h", but forgotten to stage that change for commit,
> with "git add"?
> 
> (Anyway, what was wrong with the #include directive being in
> "QemuFwCfgLib.c"?)
> 
> Apart from the #include directive, it looks good to me. (I know Liming is
> proposing a different avenue, and here I'm not arguing against that; just
> saying that under this approach, the patch seems mostly okay.)
> 
> Thanks,
> Laszlo
> 
> 
> > diff --git a/OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm
> > b/OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm
> > deleted file mode 100644
> > index faa22e9..0000000
> > --- a/OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm
> > +++ /dev/null
> > @@ -1,55 +0,0 @@
> > -;--------------------------------------------------------------------
> > ----------
> > -;
> > -; Copyright (c) 2006 - 2013, Intel Corporation. All rights
> > reserved.<BR> -; This program and the accompanying materials -; are
> > licensed and made available under the terms and conditions of the BSD
> > License -; which accompanies this distribution.  The full text of the
> > license may be found at -; http://opensource.org/licenses/bsd-
> license.php.
> > -;
> > -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS, -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > -;
> > -;--------------------------------------------------------------------
> > ----------
> > -
> > -    SECTION .text
> > -
> > -;--------------------------------------------------------------------
> > ----------
> > -;  VOID
> > -;  EFIAPI
> > -;  IoReadFifo8 (
> > -;    IN UINTN                  Port,
> > -;    IN UINTN                  Size,
> > -;    IN VOID                   *Buffer
> > -;    );
> > -;--------------------------------------------------------------------
> > ----------
> > -global ASM_PFX(IoReadFifo8)
> > -ASM_PFX(IoReadFifo8):
> > -
> > -    mov     dx, [esp + 4]
> > -    mov     ecx, [esp + 8]
> > -    push    edi
> > -    mov     edi, [esp + 16]
> > -rep insb
> > -    pop     edi
> > -    ret
> > -
> > -;--------------------------------------------------------------------
> > ----------
> > -;  VOID
> > -;  EFIAPI
> > -;  IoWriteFifo8 (
> > -;    IN UINTN                  Port,
> > -;    IN UINTN                  Size,
> > -;    IN VOID                   *Buffer
> > -;    );
> > -;--------------------------------------------------------------------
> > ----------
> > -global ASM_PFX(IoWriteFifo8)
> > -ASM_PFX(IoWriteFifo8):
> > -
> > -    mov     dx, [esp + 4]
> > -    mov     ecx, [esp + 8]
> > -    push    esi
> > -    mov     esi, [esp + 16]
> > -rep outsb
> > -    pop     esi
> > -    ret
> > -
> > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> > b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> > index 0bbf121..68fbade 100644
> > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> > @@ -2,6 +2,7 @@
> >
> >    Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.<BR>
> >    Copyright (C) 2013, Red Hat, Inc.
> > +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> >
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of
> > the BSD License @@ -26,59 +27,6 @@
> >
> >
> >  /**
> > -  Reads an 8-bit I/O port fifo into a block of memory.
> > -
> > -  Reads the 8-bit I/O fifo port specified by Port.
> > -
> > -  The port is read Count times, and the read data is
> > -  stored in the provided Buffer.
> > -
> > -  This function must guarantee that all I/O read and write operations
> > are
> > -  serialized.
> > -
> > -  If 8-bit I/O port operations are not supported, then ASSERT().
> > -
> > -  @param  Port    The I/O port to read.
> > -  @param  Count   The number of times to read I/O port.
> > -  @param  Buffer  The buffer to store the read data into.
> > -
> > -**/
> > -VOID
> > -EFIAPI
> > -IoReadFifo8 (
> > -  IN      UINTN                     Port,
> > -  IN      UINTN                     Count,
> > -  OUT     VOID                      *Buffer
> > -  );
> > -
> > -/**
> > -  Writes an 8-bit I/O port fifo from a block of memory.
> > -
> > -  Writes the 8-bit I/O fifo port specified by Port.
> > -
> > -  The port is written Count times, and the data are obtained
> > -  from the provided Buffer.
> > -
> > -  This function must guarantee that all I/O read and write operations
> > are
> > -  serialized.
> > -
> > -  If 8-bit I/O port operations are not supported, then ASSERT().
> > -
> > -  @param  Port    The I/O port to read.
> > -  @param  Count   The number of times to read I/O port.
> > -  @param  Buffer  The buffer to store the read data into.
> > -
> > -**/
> > -VOID
> > -EFIAPI
> > -IoWriteFifo8 (
> > -  IN      UINTN                     Port,
> > -  IN      UINTN                     Count,
> > -  OUT     VOID                      *Buffer
> > -  );
> > -
> > -
> > -/**
> >    Selects a firmware configuration item for reading.
> >
> >    Following this call, any data read from this item will start from
> > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> > b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> > index 66ac778..e48c639 100644
> > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> > @@ -4,6 +4,7 @@
> >  #
> >  #  Copyright (C) 2013, Red Hat, Inc.
> >  #  Copyright (c) 2008 - 2012, Intel Corporation. All rights
> > reserved.<BR>
> > +#  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> >  #
> >  #  This program and the accompanying materials  #  are licensed and
> > made available under the terms and conditions of the BSD License @@
> > -36,12 +37,6 @@ [Sources]
> >    QemuFwCfgLib.c
> >    QemuFwCfgPeiDxe.c
> >
> > -[Sources.IA32]
> > -  Ia32/IoLibExAsm.nasm
> > -
> > -[Sources.X64]
> > -  X64/IoLibExAsm.nasm
> > -
> >  [Packages]
> >    MdePkg/MdePkg.dec
> >    OvmfPkg/OvmfPkg.dec
> > @@ -51,5 +46,6 @@ [LibraryClasses]
> >    BaseMemoryLib
> >    DebugLib
> >    IoLib
> > +  IoFifoLib
> >    MemoryAllocationLib
> >
> > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> > b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> > index c1d6a54..6275ba9 100644
> > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> > @@ -4,6 +4,7 @@
> >  #
> >  #  Copyright (C) 2013, Red Hat, Inc.
> >  #  Copyright (c) 2008 - 2012, Intel Corporation. All rights
> > reserved.<BR>
> > +#  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> >  #
> >  #  This program and the accompanying materials  #  are licensed and
> > made available under the terms and conditions of the BSD License @@
> > -34,12 +35,6 @@ [Sources]
> >    QemuFwCfgLib.c
> >    QemuFwCfgSec.c
> >
> > -[Sources.IA32]
> > -  Ia32/IoLibExAsm.nasm
> > -
> > -[Sources.X64]
> > -  X64/IoLibExAsm.nasm
> > -
> >  [Packages]
> >    MdePkg/MdePkg.dec
> >    OvmfPkg/OvmfPkg.dec
> > @@ -49,5 +44,6 @@ [LibraryClasses]
> >    BaseMemoryLib
> >    DebugLib
> >    IoLib
> > +  IoFifoLib
> >    MemoryAllocationLib
> >
> > diff --git a/OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm
> > b/OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm
> > deleted file mode 100644
> > index f1078f2..0000000
> > --- a/OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm
> > +++ /dev/null
> > @@ -1,52 +0,0 @@
> > -;--------------------------------------------------------------------
> > ----------
> > -;
> > -; Copyright (c) 2006 - 2013, Intel Corporation. All rights
> > reserved.<BR> -; This program and the accompanying materials -; are
> > licensed and made available under the terms and conditions of the BSD
> > License -; which accompanies this distribution.  The full text of the
> > license may be found at -; http://opensource.org/licenses/bsd-
> license.php.
> > -;
> > -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS, -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > -;
> > -;--------------------------------------------------------------------
> > ----------
> > -
> > -    DEFAULT REL
> > -    SECTION .text
> > -
> > -;--------------------------------------------------------------------
> > ----------
> > -;  VOID
> > -;  EFIAPI
> > -;  IoReadFifo8 (
> > -;    IN UINTN                  Port,              // rcx
> > -;    IN UINTN                  Size,              // rdx
> > -;    IN VOID                   *Buffer            // r8
> > -;    );
> > -;--------------------------------------------------------------------
> > ----------
> > -global ASM_PFX(IoReadFifo8)
> > -ASM_PFX(IoReadFifo8):
> > -
> > -    xchg    rcx, rdx
> > -    xchg    rdi, r8             ; rdi: buffer address; r8: save rdi
> > -rep insb
> > -    mov     rdi, r8             ; restore rdi
> > -    ret
> > -
> > -;--------------------------------------------------------------------
> > ----------
> > -;  VOID
> > -;  EFIAPI
> > -;  IoWriteFifo8 (
> > -;    IN UINTN                  Port,              // rcx
> > -;    IN UINTN                  Size,              // rdx
> > -;    IN VOID                   *Buffer            // r8
> > -;    );
> > -;--------------------------------------------------------------------
> > ----------
> > -global ASM_PFX(IoWriteFifo8)
> > -ASM_PFX(IoWriteFifo8):
> > -
> > -    xchg    rcx, rdx
> > -    xchg    rsi, r8             ; rdi: buffer address; r8: save rdi
> > -rep outsb
> > -    mov     rsi, r8             ; restore rdi
> > -    ret
> > -
> >