Hi Sunny,
I thought about keeping the arguments the way you suggested but then decided
against it so that the auto enumerated boot options list is not tampered with
in case the function implementation hits an error in between. This way, if the
function returns an error, we can use the list we passed in and continue boot.
I am OK with any suggested name changes.
Thanks
Ashish
-----Original Message-----
From: Wang, Sunny (HPS SW) <sunnyw...@hpe.com>
Sent: Thursday, December 19, 2019 7:23 AM
To: devel@edk2.groups.io; ray...@intel.com; Wang, Jian J
<jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Gao, Zhichao
<zhichao....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
'Andrew Fish (af...@apple.com)' <af...@apple.com>
Cc: Ashish Singhal <ashishsin...@nvidia.com>; Wang, Sunny (HPS SW)
<sunnyw...@hpe.com>; Spottswood, Jason <jason.spottsw...@hpe.com>
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot
Manager Protocol
External email: Use caution opening links or attachments
1. Is it a proper solution to the problem?
Yes, it already solved my concern discussed in the other email.
2. Is the new protocol/function name proper?
Yes, but I'm not good at naming. We may need others' feedback. :) 3. Are
the parameters in the function proper?
How about we only have two parameters as below to simplify this code change?
typedef
EFI_STATUS
(EFIAPI *REFRESH_ALL_BOOT_OPTIONS) (
IN OUT EFI_BOOT_MANAGER_LOAD_OPTION **BootOptions,
IN OUT UINTN
*BootOptionsCount,
);
In addition, I have one more suggestion about the structure name inline.
Besides these two comments, everything else looks good to me.
Regards,
Sunny Wang
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Thursday, December 19, 2019 9:59 AM
To: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Gao,
Zhichao <zhichao....@intel.com>; Kinney, Michael D
<michael.d.kin...@intel.com>; 'Andrew Fish (af...@apple.com)' <af...@apple.com>
Cc: devel@edk2.groups.io; Ashish Singhal <ashishsin...@nvidia.com>
Subject: Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot
Manager Protocol
All,
The new EDKII Platform Boot Manager protocol provides a platform hook to solve
below problem.
Can you please review and think about:
1. Is it a proper solution to the problem?
2. Is the new protocol/function name proper?
3. Are the parameters in the function proper?
**Problem:
Platform requires certain BlockIo/SimpleFileSystem/LoadFile
instances don't cause Boot#### created. It's a need of platform customization.
**Details:
Boot#### for BlockIo/SimpleFileSystem/LoadFile are created by
API EfiBootMangerRefreshAllBootOptions(). There are 2 places that call this API:
1. Platform Boot Manager calls the API (usually in the full configuration
boot path)
2. UiApp calls the API when entering "Boot Manager" page and "Boot
Maintenance Manager" page.
Platform can change Platform Boot Manager library to remove the unneeded
Boot#### in case #1.
But platform has no way to remove the Boot#### created in case #2 .
Thanks,
Ray
> -----Original Message-----
> From: Ashish Singhal <ashishsin...@nvidia.com>
> Sent: Thursday, December 19, 2019 3:11 AM
> To: devel@edk2.groups.io; Wang, Jian J <jian.j.w...@intel.com>; Wu,
> Hao A <hao.a...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; Ni,
> Ray <ray...@intel.com>
> Cc: Ashish Singhal <ashishsin...@nvidia.com>
> Subject: [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager
> Protocol
>
> Add edk2 platform boot manager protocol which would have platform
> specific refreshes to the auto enumerated as well as NV boot options
> for the platform.
>
> Signed-off-by: Ashish Singhal <ashishsin...@nvidia.com>
> ---
> .../Include/Protocol/PlatformBootManager.h | 82
> ++++++++++++++++++++++
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 41 +++++++++--
> .../Library/UefiBootManagerLib/InternalBm.h | 2 +
> .../UefiBootManagerLib/UefiBootManagerLib.inf | 2 +
> MdeModulePkg/MdeModulePkg.dec | 4 ++
> 5 files changed, 124 insertions(+), 7 deletions(-) create mode
> 100644 MdeModulePkg/Include/Protocol/PlatformBootManager.h
>
> diff --git a/MdeModulePkg/Include/Protocol/PlatformBootManager.h
> b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
> new file mode 100644
> index 0000000..ec32215
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
> @@ -0,0 +1,82 @@
> +/** @file
> +
> + Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +#define __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +
> +#include <Library/UefiBootManagerLib.h>
> +
> +//
> +// Platform Boot Manager Protocol GUID value // #define
> +EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_GUID \
> + { \
> + 0xaa17add4, 0x756c, 0x460d, { 0x94, 0xb8, 0x43, 0x88, 0xd7,
> +0xfb, 0x3e,
> 0x59 } \
> + }
> +
> +//
> +// Protocol interface structure
> +//
> +typedef struct _PLATFORM_BOOT_MANAGER_PROTOCOL
> PLATFORM_BOOT_MANAGER_PROTOCOL;
For being consistent with other EDKII protocols like
EDKII_PLATFORM_LOGO_PROTOCOL , how about we update it to the following?
typedef struct _EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL
EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL;
> +
> +//
> +// Revision The revision to which the protocol interface adheres.
> +// All future revisions must be backwards compatible.
> +// If a future version is not back wards compatible it is not the
> same
> GUID.
> +//
> +#define EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION
> 0x00000001
> +
> +//
> +// Function Prototypes
> +//
> +
> +/*
> + This function allows platform to refresh all boot options specific
> +to the
> platform. Within
> + this function, platform can make modifications to the auto
> + enumerated
> platform boot options
> + as well as NV boot options.
> +
> + @param[in const] BootOptions An array of auto enumerated
> platform boot options.
> + This array will be freed by
> caller upon successful
> + exit of this function and output
> array would be used.
> +
> + @param[in const] BootOptionsCount The number of elements in
> BootOptions.
> +
> + @param[out] UpdatedBootOptions An array of boot options that
> have been customized
> + for the platform on top of input
> boot options. This
> + array would be allocated
> + by
> REFRESH_ALL_BOOT_OPTIONS
> + and would be freed by caller
> after consuming it.
> +
> + @param[out] UpdatedBootOptionsCount The number of elements in
> UpdatedBootOptions.
> +
> +
> + @retval EFI_SUCCESS Platform refresh to input
> BootOptions and
> + BootCount have been done.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @retval EFI_INVALID_PARAMETER Input is not correct.
> +
> + @retval EFI_UNSUPPORTED Platform specific overrides are
> not
> supported.
> +*/
> +typedef
> +EFI_STATUS
> +(EFIAPI *REFRESH_ALL_BOOT_OPTIONS) (
> + IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions,
> + IN CONST UINTN BootOptionsCount,
> + OUT EFI_BOOT_MANAGER_LOAD_OPTION **UpdatedBootOptions,
> + OUT UINTN *UpdatedBootOptionsCount
> + );
> +
> +struct _PLATFORM_BOOT_MANAGER_PROTOCOL {
> + UINT64 Revision;
> + REFRESH_ALL_BOOT_OPTIONS RefreshAllBootOptions; };
> +
> +extern EFI_GUID gEdkiiPlatformBootManagerProtocolGuid;
> +
> +#endif /* __PLATFORM_BOOT_MANAGER_PROTOCOL_H__ */
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index 760d764..8b9a76d 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1,6 +1,7 @@
> /** @file
> Library functions which relates with booting.
>
> +Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
> Copyright (c) 2011 - 2019, Intel Corporation. All rights
> reserved.<BR>
> (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -2258,12 +2259,15 @@
> EfiBootManagerRefreshAllBootOption (
> VOID
> )
> {
> - EFI_STATUS Status;
> - EFI_BOOT_MANAGER_LOAD_OPTION *NvBootOptions;
> - UINTN NvBootOptionCount;
> - EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
> - UINTN BootOptionCount;
> - UINTN Index;
> + EFI_STATUS Status;
> + EFI_BOOT_MANAGER_LOAD_OPTION *NvBootOptions;
> + UINTN NvBootOptionCount;
> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
> + UINTN BootOptionCount;
> + EFI_BOOT_MANAGER_LOAD_OPTION *UpdatedBootOptions;
> + UINTN UpdatedBootOptionCount;
> + UINTN Index;
> + PLATFORM_BOOT_MANAGER_PROTOCOL *PlatformBootManager;
>
> //
> // Optionally refresh the legacy boot option @@ -2273,7 +2277,6 @@
> EfiBootManagerRefreshAllBootOption (
> }
>
> BootOptions = BmEnumerateBootOptions (&BootOptionCount);
> - NvBootOptions = EfiBootManagerGetLoadOptions (&NvBootOptionCount,
> LoadOptionTypeBoot);
>
> //
> // Mark the boot option as added by BDS by setting OptionalData to
> a special GUID @@ -2284,6 +2287,30 @@
> EfiBootManagerRefreshAllBootOption (
> }
>
> //
> + // Locate Platform Boot Options Protocol // Status =
> + gBS->LocateProtocol (&gEdkiiPlatformBootManagerProtocolGuid,
> + NULL,
> + (VOID **)&PlatformBootManager); if
> + (!EFI_ERROR (Status)) {
> + //
> + // If found, call platform specific refresh to all auto enumerated and NV
> + // boot options.
> + //
> + Status = PlatformBootManager->RefreshAllBootOptions ((CONST
> EFI_BOOT_MANAGER_LOAD_OPTION *)BootOptions,
> + (CONST
> UINTN)BootOptionCount,
> + &UpdatedBootOptions,
> +
> &UpdatedBootOptionCount);
> + if (!EFI_ERROR (Status)) {
> + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
> + BootOptions = UpdatedBootOptions;
> + BootOptionCount = UpdatedBootOptionCount;
> + }
> + }
> +
> + NvBootOptions = EfiBootManagerGetLoadOptions (&NvBootOptionCount,
> LoadOptionTypeBoot);
> +
> + //
> // Remove invalid EFI boot options from NV
> //
> for (Index = 0; Index < NvBootOptionCount; Index++) { diff --git
> a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> index 027eb25..ac866ac 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> @@ -1,6 +1,7 @@
> /** @file
> BDS library definition, include the file and data structure
>
> +Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
> Copyright (c) 2004 - 2018, Intel Corporation. All rights
> reserved.<BR>
> (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -41,6 +42,7 @@
> SPDX-License-Identifier: BSD-2-Clause-Patent #include
> <Protocol/VariableLock.h> #include <Protocol/RamDisk.h> #include
> <Protocol/DeferredImageLoad.h>
> +#include <Protocol/PlatformBootManager.h>
>
> #include <Guid/MemoryTypeInformation.h> #include <Guid/FileInfo.h>
> diff --git
> a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> index ed6b467..cf59086 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> +++
> b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> @@ -5,6 +5,7 @@
> # manipulation, hotkey registration, UEFI boot, connect/disconnect,
> console # manipulation, driver health checking and etc.
> #
> +# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
> # Copyright (c) 2007 - 2018, Intel Corporation. All rights
> reserved.<BR> # (C) Copyright 2016 Hewlett Packard Enterprise
> Development LP<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent @@
> -107,6 +108,7 @@
> gEfiFormBrowser2ProtocolGuid ## SOMETIMES_CONSUMES
> gEfiRamDiskProtocolGuid ## SOMETIMES_CONSUMES
> gEfiDeferredImageLoadProtocolGuid ## SOMETIMES_CONSUMES
> + gEdkiiPlatformBootManagerProtocolGuid ## SOMETIMES_CONSUMES
>
> [Pcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationC
> hange ## SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 41b9e70..cc238e9 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -3,6 +3,7 @@
> # It also provides the definitions(including PPIs/PROTOCOLs/GUIDs and
> library classes) # and libraries instances, which are used for those
> modules.
> #
> +# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
> # Copyright (c) 2007 - 2019, Intel Corporation. All rights
> reserved.<BR> # Copyright (c) 2016, Linaro Ltd. All rights
> reserved.<BR> # (C) Copyright 2016 - 2019 Hewlett Packard Enterprise
> Development LP<BR> @@ -609,6 +610,9 @@
> ## Include/Protocol/PeCoffImageEmulator.h
> gEdkiiPeCoffImageEmulatorProtocolGuid = { 0x96f46153, 0x97a7,
> 0x4793, { 0xac, 0xc1, 0xfa, 0x19, 0xbf, 0x78, 0xea, 0x97 } }
>
> + ## Include/Protocol/PlatformBootManager.h
> + gEdkiiPlatformBootManagerProtocolGuid = { 0xaa17add4, 0x756c,
> + 0x460d,
> { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 0x59 } }
> +
> #
> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> # 0x80000001 | Invalid value provided.
> --
> 2.7.4
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information. Any unauthorized review, use, disclosure or
distribution
is prohibited. If you are not the intended recipient, please contact the
sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52431): https://edk2.groups.io/g/devel/message/52431
Mute This Topic: https://groups.io/mt/68802855/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
