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

[Laszlo Ersek <lersek@redhat.com>]

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