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
