I found it in my junk folder. -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Friday, August 14, 2015 8:20 PM To: Ni, Ruiyu <ruiyu...@intel.com> Cc: Justen, Jordan L <jordan.l.jus...@intel.com>; edk2-de...@ml01.01.org Subject: Re: [edk2] [Patch 2/3] OvmfPkg: use new BDS and UiApp in MdeModulePkg
On 08/14/15 10:28, Ni, Ruiyu wrote: > Laszlo, > Where can I read your first 17 remarks? I didn't find it in my mail folder. Strange; your email address <ruiyu...@intel.com> was the only one in the To: field. (The list and Jordan were Cc'd.) In any case, here's the link into the archive: http://thread.gmane.org/gmane.comp.bios.edk2.devel/759/focus=1153 Thanks! Laszlo > > Thanks, > Ray > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, August 12, 2015 10:59 PM > To: Ni, Ruiyu <ruiyu...@intel.com> > Cc: Justen, Jordan L <jordan.l.jus...@intel.com>; edk2-de...@ml01.01.org > Subject: Re: [edk2] [Patch 2/3] OvmfPkg: use new BDS and UiApp in MdeModulePkg > > Continuing: > > On 08/12/15 00:53, Laszlo Ersek wrote: >> On 08/03/15 07:41, Ruiyu Ni wrote: >>> Compare to the old BDS, the new BDS separates the UI part to a standalone >>> application UiApp. >>> QemuBootOrderLib was changed to depend on the UefiBootManagerLib. > >> I've covered the following files thus far: >>> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c >>> | 347 +++++++---- >>> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >>> | 4 +- >>> OvmfPkg/OvmfPkg.dec >>> | 5 +- >>> OvmfPkg/OvmfPkgIa32.dsc >>> | 28 +- >>> OvmfPkg/OvmfPkgIa32.fdf >>> | 3 +- >>> OvmfPkg/OvmfPkgIa32X64.dsc >>> | 30 +- >>> OvmfPkg/OvmfPkgIa32X64.fdf >>> | 3 +- >>> OvmfPkg/OvmfPkgX64.dsc >>> | 28 +- >>> OvmfPkg/OvmfPkgX64.fdf >>> | 3 +- >> >> and made 17 remarks that should be addressed in v2. >> >> I will continue the review later; the rest of the patch is preserved in >> the trailing context, so I will follow up on that. The remaining >> diffstat is, with rename & copy detection enabled: >> > > (sorting the below) > >>> EdkCompatibilityPkg/Foundation/Library/Dxe/GraphicsLite/Graphics.c => >>> OvmfPkg/Library/PlatformBootManagerLib/QuietBoot.c | 559 +++++++++--------- >>> OvmfPkg/Library/PlatformBootManagerLib/Strings.uni >>> | Bin 0 -> 3658 bytes >>> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/BdsPlatform.c >>> | 611 ++++++++------------ >>> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/BdsPlatform.h >>> | 132 ++--- >>> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/PlatformData.c >>> | 18 +- >>> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/QemuKernel.c >>> | 0 >>> OvmfPkg/Library/{PlatformBdsLib/PlatformBdsLib.inf => >>> PlatformBootManagerLib/PlatformBootManagerLib.inf} | 20 +- >>> {IntelFrameworkModulePkg/Universal/BdsDxe => >>> OvmfPkg/Library/PlatformBootManagerLib}/MemoryTest.c >>> | 227 ++++---- > > Let's see "MemoryTest.c" and "Strings.uni" first. > >>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/MemoryTest.c >>> b/OvmfPkg/Library/PlatformBootManagerLib/MemoryTest.c >>> new file mode 100644 >>> index 0000000..c9a7ecb >>> --- /dev/null >>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/MemoryTest.c > > [contents snipped] > >>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/Strings.uni >>> b/OvmfPkg/Library/PlatformBootManagerLib/Strings.uni >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..7300975620fef86ea31c556a6fa66c098e8a0538 >>> GIT binary patch >>> literal 3658 > > [contents snipped] > > These two files do the following: > > - "MemoryTest.c" is a slightly customized copy of > "IntelFrameworkModulePkg/Universal/BdsDxe/MemoryTest.c". The > customization comprises: > > - hard-coding PcdBootlogoOnlyEnable as FALSE > - removing DEBUG messages > - open coding some HII string lookup helper functions > > - "Strings.uni" provides English and French text for the following > string tokens: > > - STR_PERFORM_MEM_TEST, > - STR_MEMORY_TEST_PERCENT, > - STR_ESC_TO_SKIP_MEM_TEST, > - STR_MEM_TEST_COMPLETED, > - STR_NO_EXT_MEM_FOUND, > - STR_SYSTEM_MEM_ERROR > > All of these tokens are needed for messages printed by "MemoryTest.c". > > (18) Now that I understand what these files do: please drop them both. > There's no need for them, for the following reasons: > > - They complicate OvmfPkg for no benefit. > > - The memory test for a virtual machine is a joke. We don't do it in > ArmVirtPkg, for example. > > - Dropping these files -- and therefore the BdsMemoryTest() function -- > only causes a *minimal* visual change on the splash screen. The amount > of total memory won't be printed, and the (completely useless, > immediately "full") memory test progress bar at the bottom will > disappear. > > That's all fine. The amount of memory can be queried with the "memmap" > UEFI shell command (it prints a summary and a grand total after the > UEFI memmap). I verified it. > > I've always wanted to get rid of this fake memory test. (Note the > protocol provider: MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe.) > This is a good opportunity to do it. It will simplify both this patch, > and OvmfPkg in general. > > Should we decide later that we "absolutely" need this thing, we can > always add it as a separate step. So, thank you for being careful and > trying to preserve this feature, but it's a mis-feature, and we should > cut it. > > So please drop these two files, plus remove the BdsMemoryTest() function > declaration from "BdsPlatform.h", and the BdsMemoryTest() function calls > from "BdsPlatform.c". (I'll repeat these points separately, when > reviewing those files.) > > On to the next file: > >>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QuietBoot.c >>> b/OvmfPkg/Library/PlatformBootManagerLib/QuietBoot.c >>> new file mode 100644 >>> index 0000000..13c0caa >>> --- /dev/null >>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/QuietBoot.c > > [contents snipped] > > So, the git copy detector works with a numeric similarity index. For > that reason, it so happens that git thinks this file is a modified copy > of "EdkCompatibilityPkg/Foundation/Library/Dxe/GraphicsLite/Graphics.c". > > That's clearly not the case. This copy comes from > "IntelFrameworkModulePkg/Library/GenericBdsLib/BdsConsole.c". > > Many functions from that file have been dropped. But, the following > functions have been preserved: > - ConvertBmpToGopBlt() > - EnableQuietBoot() > - DisableQuietBoot() > > The only changes in them are: > - some new #include directives > - gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable > hardcoded as FALSE -- this is the default value in the DEC file, and > OVMF doesn't change it, so it's fine > - dropped EFIAPI calling convention specifiers -- that's fine, these > functions are not an external interface any longer. > > I agree that these functions should be preserved (they are needed for > putting up the TianoCore logo, and that's something we want to keep.) > > It's also fine (for now) that the file has overlong lines; it's a > (partial) copy of another file (and it is kept separate under OvmfPkg > too), so that's okay. > > However, please implement the following changes: > > (19) Please create "QuietBoot.c" in a separate patch, *before* this > patch. It doesn't have to be added to any INF file, and at that stage it > doesn't need to be compiled. > > But, it should be split out for size reasons, and you should definitely > explain in the commit message where it comes from, and why those three > functions are preserved. > > When I run "git log -- .../QuietBoot.c" in a year or two, with > GenericBdsLib long gone by then, I'd like to be reminded that > QuietBoot.c comes from GenericBdsLib. > > (20) I notice that you forgot to remove EFIAPI from DisableQuietBoot(); > please fix that for consistency's sake. > > (21) Please make ConvertBmpToGopBlt() STATIC. > > > The next file, "PlatformBootManagerLib.inf", is a modified copy of > "PlatformBdsLib.inf". Therefore I'm going to replace the patch for that > file in your email, and review the copy+diff instead: > >> diff --git a/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf >> b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> similarity index 71% >> copy from OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf >> copy to OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> index ab54683..1e3c3a2 100644 >> --- a/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> @@ -1,7 +1,7 @@ >> ## @file >> # Platform BDS customizations library. >> # >> -# Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR> >> +# Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> > > (22) Since this is obviously derivative work, please don't replace the > copyright notice. The previous copyright notice is actually just right. > >> # 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 >> @@ -14,23 +14,26 @@ >> >> [Defines] >> INF_VERSION = 0x00010005 >> - BASE_NAME = PlatformBdsLib >> - FILE_GUID = F844172E-9985-44f2-BADE-0DD783462E95 >> + BASE_NAME = PlatformBootManagerLib >> + FILE_GUID = FB65006C-AC9F-4992-AD80-184B2BDBBD83 >> MODULE_TYPE = DXE_DRIVER >> VERSION_STRING = 1.0 >> - LIBRARY_CLASS = PlatformBdsLib|DXE_DRIVER >> + LIBRARY_CLASS = PlatformBootManagerLib|DXE_DRIVER >> >> # >> # The following information is for reference only and not required by the >> build tools. >> # >> -# VALID_ARCHITECTURES = IA32 X64 IPF EBC >> +# VALID_ARCHITECTURES = IA32 X64 >> # >> >> [Sources] >> BdsPlatform.c >> PlatformData.c >> QemuKernel.c >> + MemoryTest.c > > (23) So "MemoryTest.c" is not necessary. (Please see (18).) > >> + QuietBoot.c > > Okay. > >> BdsPlatform.h >> + Strings.uni > > (24) "Strings.uni" should be removed as well, for the same reason. > >> >> [Packages] >> MdePkg/MdePkg.dec >> @@ -45,8 +48,8 @@ [LibraryClasses] >> BaseMemoryLib >> DebugLib >> PcdLib >> - GenericBdsLib >> PciLib >> + UefiBootManagerLib >> NvVarsFileLib >> QemuFwCfgLib >> LoadLinuxLib > > Good. > >> @@ -58,6 +61,9 @@ [Pcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId >> + gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut >> + gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## >> CONSUMES >> + gUefiOvmfPkgTokenSpaceGuid.PcdShellFile >> >> [Pcd.IA32, Pcd.X64] >> gEfiMdePkgTokenSpaceGuid.PcdFSBClock >> @@ -67,6 +73,8 @@ [Protocols] >> gEfiPciRootBridgeIoProtocolGuid >> gEfiS3SaveStateProtocolGuid # PROTOCOL >> SOMETIMES_CONSUMED >> gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL >> SOMETIMES_PRODUCED >> + gEfiOEMBadgingProtocolGuid > > (25) This is fine (it comes with "QuietBoot.c"), but you should preserve > the protocol usage comment from > "IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf", which > is: "## SOMETIMES_CONSUMES". > > ... For consistency with the rest of the INF file here, I think you > should add "# PROTOCOL SOMETIMES_CONSUMED" actually. > >> + gEfiGenericMemTestProtocolGuid > > (26) Please remove this (see (18)). > >> >> [Guids] >> gEfiEndOfDxeEventGroupGuid > > Next file: > >>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c >>> b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c >>> new file mode 100644 >>> index 0000000..ef728df >>> --- /dev/null >>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c > > [contents snipped] > > This file is an identical copy of > "OvmfPkg/Library/PlatformBdsLib/QemuKernel.c". Which is alright, but: > > (27) it should be split out to a standalone patch (with nothing else in > it), and the commit message should state where it comes from, and that > there are no changes relative to the original file. > > > Finally, three files remain: > >> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/BdsPlatform.c >> | 611 ++++++++------------ >> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/BdsPlatform.h >> | 132 ++--- >> OvmfPkg/Library/{PlatformBdsLib => PlatformBootManagerLib}/PlatformData.c >> | 18 +- > > (28) I tried to understand the changes to these files, but there are > simply too many. It seems to be a big bag of unrelated changes mixed > together. > > Please, split up the changes to these files to *several* patches. > Practically, a sub-series of patches. Each patch should contain a > minimal, logically indivisible change, and a *detailed* commit message > as to why that change is necessary and correct. > > It is clear to me that while this is being done *gradually*, in small > steps, the new library will not compile. That's 100% fine: until you are > done with the final changes, please simply do not flip OvmfPkg (the DEC, > DSC, FDF files) over to the new library. > > The *new* code does not have to compile (nor be in use at all) until > everything is ready. However, please do build up the new code in small > logical steps, so that we have precise documentation (commit messages) > for each atomic logical change. While that is in progress, the old code > will continue to build and work just fine. Once everything is in place, > the switch can be flipped to the new library, the new driver, and the > new application, in the DEC / DSC / FDF files. > > As-is, these three last files are unreviewable to me. > > Ultimately, this one patch should be split up to many small patches. > Please try to advance in as small logical steps as possible, and write > extensive commit messages for all of them. > > I'm looking forward to reviewing version 2! > > Thanks! > Laszlo > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
