Re: [PATCH 0/4] *** BaseIoFifoLib ***

[Laszlo Ersek <lersek@redhat.com>]

On 01/05/17 00:07, Leo Duran wrote:
> 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 a common BaseIofifoLib library supporting IA32 and X64
> architectures under MdePkg.
> 
> The intent of this patch series is twofold:
> 1) Consolidate I/O Fifo routines into a common BaseIofifoLib library.
> 2) Allow override of BaseIofifoLib for specific platform implementations.
> For example, the OVMF package can provide its own version of BaseIoFifoLib
> to support Secure Encrypted Virtualization (SEV) guests, since SEV does not
> support string I/O instructions (rep ins/outs), and requires unrolled loops
> of single in/out instructions.

General comments:

(1) Please rebase the series, picking "edit" for each patch, and
re-commit each patch (without any code changes) with:

  git commit --amend --author='Brijesh Singh <brijesh.singh@amd.com>'

This will cause the formatted patch emails to start with

From: Brijesh Singh <brijesh.singh@amd.com>

and Brijesh's authorship will be then correctly reflected in git
metadata after applying the patches with "git am".

(I'm basing this on the Signed-off-by tags.)

(2) I think your S-o-b should also be present, at the bottom.

(3) If you updated the patches after getting them from Brijesh (i.e.,
you aren't merely forwarding them), then I believe you should drop your
Reviewed-by.

(4) The new files shouldn't carry just Intel's copyright notice -- they
are new files, so they should get AMD's too. (Some of the new files do
this, some don't.)

Comments related to virt:

(5) OVMF now supports (and prefers) the DMA-like interface of fw_cfg
(available in newer QEMU releases), which doesn't use port IO at all; so
in practice the REPs should cause problems under SEV, when used in
QemuFwCfgLib.

Nonetheless, I do agree that code deduplication is a good thing!

(6) Single IO port instructions in CpuIo2Dxe will slow down emulated IDE
transfers considerably. (I'm not holding this against the lib class
extraction or SEV in general. We should simply acknowledge this as
another reason to default to virtio-blk or virtio-scsi in VM configs.)

Thanks!
Laszlo


> 
> Leo Duran (4):
>   MdePkg: Add BaseIoFifoLib library
>   Modify .DSC files that include UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>   Modify UefiCpuPkg/CpuIo2Dxe to use new BaseIoFifoLib library.
>   Modify QemuFwCfgLib to use new BaseIoFifoLib library.
> 
>  CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc     |   1 +
>  CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc  |   1 +
>  DuetPkg/DuetPkgIa32.dsc                           |   1 +
>  DuetPkg/DuetPkgX64.dsc                            |   1 +
>  MdePkg/Include/Library/IoFifoLib.h                | 175 ++++++++++++++++++++++
>  MdePkg/Library/BaseIoFifoLib/BaseIoFifoLib.inf    |  45 ++++++
>  MdePkg/Library/BaseIoFifoLib/Ia32/IoFifo.asm      | 139 +++++++++++++++++
>  MdePkg/Library/BaseIoFifoLib/Ia32/IoFifo.nasm     | 135 +++++++++++++++++
>  MdePkg/Library/BaseIoFifoLib/X64/IoFifo.asm       | 125 ++++++++++++++++
>  MdePkg/Library/BaseIoFifoLib/X64/IoFifo.nasm      | 124 +++++++++++++++
>  MdePkg/MdePkg.dec                                 |   3 +
>  MdePkg/MdePkg.dsc                                 |   1 +
>  OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm |  55 -------
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c       |   1 +
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf     |   7 +-
>  OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm  |  52 -------
>  OvmfPkg/OvmfPkgIa32.dsc                           |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                        |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                            |   1 +
>  QuarkPlatformPkg/Quark.dsc                        |   1 +
>  QuarkPlatformPkg/QuarkMin.dsc                     |   1 +
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c                  |   1 -
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.h                  |   1 +
>  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf                |  10 +-
>  UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.asm              | 140 -----------------
>  UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.nasm             | 136 -----------------
>  UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.asm               | 126 ----------------
>  UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.nasm              | 125 ----------------
>  UefiCpuPkg/UefiCpuPkg.dsc                         |   1 +
>  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc           |   1 +
>  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc             |   1 +
>  Vlv2TbltDevicePkg/PlatformPkgX64.dsc              |   1 +
>  32 files changed, 764 insertions(+), 650 deletions(-)
>  create mode 100644 MdePkg/Include/Library/IoFifoLib.h
>  create mode 100644 MdePkg/Library/BaseIoFifoLib/BaseIoFifoLib.inf
>  create mode 100644 MdePkg/Library/BaseIoFifoLib/Ia32/IoFifo.asm
>  create mode 100644 MdePkg/Library/BaseIoFifoLib/Ia32/IoFifo.nasm
>  create mode 100644 MdePkg/Library/BaseIoFifoLib/X64/IoFifo.asm
>  create mode 100644 MdePkg/Library/BaseIoFifoLib/X64/IoFifo.nasm
>  delete mode 100644 OvmfPkg/Library/QemuFwCfgLib/Ia32/IoLibExAsm.nasm
>  delete mode 100644 OvmfPkg/Library/QemuFwCfgLib/X64/IoLibExAsm.nasm
>  delete mode 100644 UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.asm
>  delete mode 100644 UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.nasm
>  delete mode 100644 UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.asm
>  delete mode 100644 UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.nasm
>