This patch looks very good, I have a few minor comments.
(Sorry that I'm taking so long to review individual patches -- reviewing this
one, and writing up my comments for it, took me two hours, for example. I'm
trying to be thorough.)
On 04/21/16 08:57, Ruiyu Ni wrote:
> NOTE: SetBootOrderFromQemu() interface is not changed.
> But when the old IntelFrameworkModulePkg/BDS is no longer used in
> OVMF and ArmVirtPkg, additional patch will be submitted to change
> this interface to remove parameter BootOptionList.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> ---
> .../Library/QemuNewBootOrderLib/QemuBootOrderLib.c | 134
> ++++++++++++---------
> .../QemuNewBootOrderLib/QemuBootOrderLib.inf | 4 +-
> 2 files changed, 80 insertions(+), 58 deletions(-)
>
> diff --git a/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.c
> b/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.c
> index 15065b7..f015422 100644
> --- a/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.c
> +++ b/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.c
> @@ -2,7 +2,7 @@
> Rewrite the BootOrder NvVar based on QEMU's "bootorder" fw_cfg file.
>
> Copyright (C) 2012 - 2014, Red Hat, Inc.
> - Copyright (c) 2013, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -13,10 +13,11 @@
> WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> **/
>
> +#include <Protocol/BlockIo.h>
(1) Why is this protocol include necessary?
> #include <Library/QemuFwCfgLib.h>
> #include <Library/DebugLib.h>
> #include <Library/MemoryAllocationLib.h>
> -#include <Library/GenericBdsLib.h>
> +#include <Library/UefiBootManagerLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiRuntimeServicesTableLib.h>
> #include <Library/BaseLib.h>
> @@ -253,8 +254,10 @@ typedef struct {
> LOAD_OPTION_ACTIVE attribute.
> **/
> typedef struct {
> - CONST BDS_COMMON_OPTION *BootOption; // reference only, no ownership
> - BOOLEAN Appended; // has been added to a BOOT_ORDER?
> + CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOption; // reference only, no
> + // ownership
> + BOOLEAN Appended; // has been added to a
> + // BOOT_ORDER?
> } ACTIVE_OPTION;
>
>
> @@ -300,7 +303,7 @@ BootOrderAppend (
> }
>
> BootOrder->Data[BootOrder->Produced++] =
> -
> ActiveOption->BootOption->BootCurrent;
> + (UINT16)
> ActiveOption->BootOption->OptionNumber;
I remember that BDS_COMMON_OPTION.OptionNumber also exists, but it wasn't doing
the right thing; I had to use BootCurrent.
I briefly checked EFI_BOOT_MANAGER_LOAD_OPTION.OptionNumber, and apparently it
should do the right thing -- tell us what Boot#### the boot option comes from.
(2) Can you please confirm that?
> ActiveOption->Appended = TRUE;
> return RETURN_SUCCESS;
> }
> @@ -310,14 +313,16 @@ BootOrderAppend (
>
> Create an array of ACTIVE_OPTION elements for a boot option list.
(3) Please update the heading to say "boot option array".
> - @param[in] BootOptionList A boot option list, created with
> - BdsLibEnumerateAllBootOption().
> + @param[in] BootOptions A boot option array, created by
> + EfiBootManagerGetLoadOptions ().
Hmm, this is not (entirely) correct.
I understand that technically, the boot option array will be created for this
function with EfiBootManagerGetLoadOptions(). So that part of the comment is
okay.
However, it is important to document that the boot option array passed to this
function must be complete -- in other words, it must contain the output of
full, automatic boot option generation.
The point is that QemuBootOrderLib can only filter out and reorder boot
options; it cannot create boot options. It relies on the generic BDS library to
auto-generate all possible boot options (for example, for new virtual devices
that have been configured since the last run of the virtual machine). Then it
will decide, case-by-case, whether each of those auto-generated boot options
should be preserved (and at what position), or removed.
At the moment, we have the following call chain:
BdsEntry()
[IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c]
// create empty list
InitializeListHead (&BootOptionList)
PlatformBdsPolicyBehavior()
[OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c]
BdsLibConnectAll()
[IntelFrameworkModulePkg/Library/GenericBdsLib/BdsConnect.c]
BdsLibEnumerateAllBootOption()
[IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c]
// at this point, BootOptionList
// ends with the auto-generated
// options
SetBootOrderFromQemu()
[OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
CollectActiveOptions()
[OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
which satisfies this requirement.
Skippig a bit forward in your series, I see that we end up with the following
call tree:
PlatformBootManagerAfterConsole()
[OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
PlatformBdsConnectSequence()
[OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
EfiBootManagerConnectAll()
[MdeModulePkg/Library/UefiBootManagerLib/BmConnect.c]
EfiBootManagerRefreshAllBootOption()
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
// at this point all auto-generated
// options should exist at the end
// of BootOrder
SetBootOrderFromQemu()
[OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.c]
EfiBootManagerGetLoadOptions()
[MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c]
// BootOptions contains everyting
// from BootOrder
CollectActiveOptions()
[OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.c]
(4) So ultimately, the conversion of this part (throughout the series) looks
safe, but the documentation should also mention
EfiBootManagerRefreshAllBootOption(), something like:
@param[in] BootOptions A boot option array, created with
EfiBootManagerRefreshAllBootOption () and
EfiBootManagerGetLoadOptions ().
>
> - @param[out] ActiveOption Pointer to the first element in the new array.
> - The caller is responsible for freeing the array
> - with FreePool() after use.
> + @param[in] BootOptionCount The number of elements in BootOptions.
>
> - @param[out] Count Number of elements in the new array.
> + @param[out] ActiveOption Pointer to the first element in the new array.
> + The caller is responsible for freeing the
> array
> + with FreePool() after use.
> +
> + @param[out] Count Number of elements in the new array.
>
>
> @retval RETURN_SUCCESS The ActiveOption array has been created.
> @@ -331,11 +336,13 @@ BootOrderAppend (
> STATIC
> RETURN_STATUS
> CollectActiveOptions (
> - IN CONST LIST_ENTRY *BootOptionList,
> - OUT ACTIVE_OPTION **ActiveOption,
> - OUT UINTN *Count
> + IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions,
> + IN UINTN BootOptionCount,
> + OUT ACTIVE_OPTION **ActiveOption,
> + OUT UINTN *Count
> )
> {
> + UINTN Index;
> UINTN ScanMode;
>
> *ActiveOption = NULL;
> @@ -346,22 +353,15 @@ CollectActiveOptions (
> // - store links to active entries.
> //
> for (ScanMode = 0; ScanMode < 2; ++ScanMode) {
> - CONST LIST_ENTRY *Link;
> -
> - Link = BootOptionList->ForwardLink;
> *Count = 0;
> - while (Link != BootOptionList) {
> - CONST BDS_COMMON_OPTION *Current;
> -
> - Current = CR (Link, BDS_COMMON_OPTION, Link,
> BDS_LOAD_OPTION_SIGNATURE);
> - if (IS_LOAD_OPTION_TYPE (Current->Attribute, LOAD_OPTION_ACTIVE)) {
> + for (Index = 0; Index < BootOptionCount; Index++) {
> + if ((BootOptions[Index].Attributes & LOAD_OPTION_ACTIVE) != 0) {
> if (ScanMode == 1) {
> - (*ActiveOption)[*Count].BootOption = Current;
> + (*ActiveOption)[*Count].BootOption = &BootOptions[Index];
> (*ActiveOption)[*Count].Appended = FALSE;
> }
> ++*Count;
> }
> - Link = Link->ForwardLink;
> }
>
> if (ScanMode == 0) {
> @@ -1440,8 +1440,12 @@ Match (
> IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath
> )
> {
> - CHAR16 *Converted;
> - BOOLEAN Result;
> + CHAR16 *Converted;
> + BOOLEAN Result;
> + VOID *FileBuffer;
> + UINTN FileSize;
> + EFI_DEVICE_PATH_PROTOCOL *AbsDevicePath;
> + CHAR16 *AbsConverted;
>
> Converted = ConvertDevicePathToText (
> DevicePath,
> @@ -1454,31 +1458,26 @@ Match (
>
> //
> // Attempt to expand any relative UEFI device path starting with HD() to an
> - // absolute device path first. The logic imitates
> BdsLibBootViaBootOption().
> - // We don't have to free the absolute device path,
> - // BdsExpandPartitionPartialDevicePathToFull() has internal caching.
> + // absolute device path first.
> //
> - Result = FALSE;
(5) This assignment should not be removed -- otherwise, if
EfiBootManagerGetLoadOptionBuffer() fails below, we jump to the Exit label with
Result uninitialized.
> - if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
> - DevicePathSubType (DevicePath) == MEDIA_HARDDRIVE_DP) {
> - EFI_DEVICE_PATH_PROTOCOL *AbsDevicePath;
> - CHAR16 *AbsConverted;
> -
> - AbsDevicePath = BdsExpandPartitionPartialDevicePathToFull (
> - (HARDDRIVE_DEVICE_PATH *) DevicePath);
> - if (AbsDevicePath == NULL) {
> - goto Exit;
> - }
> - AbsConverted = ConvertDevicePathToText (AbsDevicePath, FALSE, FALSE);
> - if (AbsConverted == NULL) {
> - goto Exit;
> - }
> - DEBUG ((DEBUG_VERBOSE,
> - "%a: expanded relative device path \"%s\" for prefix matching\n",
> - __FUNCTION__, Converted));
> - FreePool (Converted);
> - Converted = AbsConverted;
> + FileBuffer = EfiBootManagerGetLoadOptionBuffer (
> + (EFI_DEVICE_PATH_PROTOCOL *) DevicePath,
(6) I think that this cast is only necessary because
EfiBootManagerGetLoadOptionBuffer() does not accept a pointer-to-CONST here.
What do you think about modifying EfiBootManagerGetLoadOptionBuffer() instead?
(It could be done in patch #1.)
If it is very messy (= would need a lot of changes), then I agree that this
cast is okay. After all, the FilePath param of
EfiBootManagerGetLoadOptionBuffer() is "IN", so we can trust it not to modify
FilePath.
> + &AbsDevicePath, &FileSize
> + );
> + if (FileBuffer == NULL) {
> + goto Exit;
> + }
> + FreePool (FileBuffer);
> + AbsConverted = ConvertDevicePathToText (AbsDevicePath, FALSE, FALSE);
> + FreePool (AbsDevicePath);
> + if (AbsConverted == NULL) {
> + goto Exit;
> }
> + DEBUG ((DEBUG_VERBOSE,
> + "%a: expanded relative device path \"%s\" for prefix matching\n",
(7) This looks great, thanks. Since we are doing the conversion unconditionally
now, can you please replace "relative" with "possibly relative" in the debug
message? (It will fit on a single line, I checked.)
> + __FUNCTION__, Converted));
> + FreePool (Converted);
> + Converted = AbsConverted;
>
> //
> // Is Translated a prefix of Converted?
> @@ -1538,11 +1537,11 @@ BootOrderComplete (
> Idx = 0;
> while (!RETURN_ERROR (Status) && Idx < ActiveCount) {
> if (!ActiveOption[Idx].Appended) {
> - CONST BDS_COMMON_OPTION *Current;
> - CONST EFI_DEVICE_PATH_PROTOCOL *FirstNode;
> + CONST EFI_BOOT_MANAGER_LOAD_OPTION *Current;
> + CONST EFI_DEVICE_PATH_PROTOCOL *FirstNode;
>
> Current = ActiveOption[Idx].BootOption;
> - FirstNode = Current->DevicePath;
> + FirstNode = Current->FilePath;
> if (FirstNode != NULL) {
> CHAR16 *Converted;
> STATIC CHAR16 ConvFallBack[] = L"<unable to convert>";
> @@ -1635,7 +1634,7 @@ PruneBootVariables (
> CHAR16 VariableName[9];
>
> UnicodeSPrintAsciiFormat (VariableName, sizeof VariableName,
> "Boot%04x",
> - ActiveOption[Idx].BootOption->BootCurrent);
> + ActiveOption[Idx].BootOption->OptionNumber);
>
> //
> // "The space consumed by the deleted variable may not be available
> until
> @@ -1699,6 +1698,17 @@ SetBootOrderFromQemu (
>
> UINTN TranslatedSize;
> CHAR16 Translated[TRANSLATION_OUTPUT_SIZE];
> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
> + UINTN BootOptionCount;
> +
> + //
> + // The QemuBootOrderLib is linked by OvmfPkg and ArmVirtPkg.
> + // OvmfPkg was changed to use the new BDS @ MdeModulePkg, so boot options
> + // are no longer stored in linked list.
> + // But we don't change the QemuBootOrderLib class interface because
> + // ArmVirtPkg are still using old BDS @ IntelFrameworkModulePkg.
> + //
> + ASSERT (BootOptionList == NULL);
>
> Status = QemuFwCfgFindFile ("bootorder", &FwCfgItem, &FwCfgSize);
> if (Status != RETURN_SUCCESS) {
> @@ -1736,11 +1746,20 @@ SetBootOrderFromQemu (
> goto ErrorFreeFwCfg;
> }
>
> - Status = CollectActiveOptions (BootOptionList, &ActiveOption,
> &ActiveCount);
> - if (RETURN_ERROR (Status)) {
> + BootOptions = EfiBootManagerGetLoadOptions (
> + &BootOptionCount, LoadOptionTypeBoot
> + );
> + if (BootOptions == NULL) {
> goto ErrorFreeBootOrder;
(8) If this jump is taken, then Status will not be set correctly.
Thus, please set Status to RETURN_NOT_FOUND before the goto statement.
> }
>
> + Status = CollectActiveOptions (
> + BootOptions, BootOptionCount, &ActiveOption, &ActiveCount
> + );
> + if (RETURN_ERROR (Status)) {
> + goto ErrorFreeBootOptions;
> + }
> +
> if (FeaturePcdGet (PcdQemuBootOrderPciTranslation)) {
> Status = CreateExtraRootBusMap (&ExtraPciRoots);
> if (EFI_ERROR (Status)) {
> @@ -1770,7 +1789,7 @@ SetBootOrderFromQemu (
> if (Match (
> Translated,
> TranslatedSize, // contains length, not size, in CHAR16's here
> - ActiveOption[Idx].BootOption->DevicePath
> + ActiveOption[Idx].BootOption->FilePath
> )
> ) {
> //
> @@ -1831,6 +1850,9 @@ ErrorFreeExtraPciRoots:
> ErrorFreeActiveOption:
> FreePool (ActiveOption);
>
> +ErrorFreeBootOptions:
> + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
> +
> ErrorFreeBootOrder:
> FreePool (BootOrder.Data);
>
> diff --git a/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.inf
> b/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.inf
> index 1024328..16aea08 100644
> --- a/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.inf
> +++ b/OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.inf
> @@ -36,14 +36,14 @@ [Sources]
>
> [Packages]
> MdePkg/MdePkg.dec
> - IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> QemuFwCfgLib
> DebugLib
> MemoryAllocationLib
> - GenericBdsLib
> + UefiBootManagerLib
> UefiBootServicesTableLib
> UefiRuntimeServicesTableLib
> BaseLib
>
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
