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

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

Lazlo,
Thanks for the thorough feedback... I just sent out v2.
Leo.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, January 05, 2017 11:25 AM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: Singh, Brijesh <brijesh.singh@amd.com>; liming.gao@intel.com;
> michael.d.kinney@intel.com; jeff.fan@intel.com
> Subject: Re: [edk2] [PATCH 0/4] *** BaseIoFifoLib ***
> 
> 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
> >