From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 83B6C81998 for ; Thu, 5 Jan 2017 09:24:43 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D1CF881243; Thu, 5 Jan 2017 17:24:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-100.phx2.redhat.com [10.3.116.100]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v05HOfHZ030488; Thu, 5 Jan 2017 12:24:42 -0500 To: Leo Duran , edk2-devel@ml01.01.org References: <1483571273-11187-1-git-send-email-leo.duran@amd.com> Cc: brijesh.singh@amd.com, liming.gao@intel.com, michael.d.kinney@intel.com, jeff.fan@intel.com From: Laszlo Ersek Message-ID: <6f0fe479-96d1-9c86-2a93-27f4f9dc2cef@redhat.com> Date: Thu, 5 Jan 2017 18:24:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1483571273-11187-1-git-send-email-leo.duran@amd.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 05 Jan 2017 17:24:44 +0000 (UTC) Subject: Re: [PATCH 0/4] *** BaseIoFifoLib *** X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jan 2017 17:24:43 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 ' This will cause the formatted patch emails to start with From: Brijesh Singh 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 >