OK. I am not familiar with PCD, it is new usage for me. And now I got to know the reason. But seems the behavior would be different base on the initialization on DSC file. Whatever, this patch is OK to me. Reviewed-by: Zhichao Gao <zhichao....@intel.com>
Thanks, Zhichao > -----Original Message----- > From: Grzegorz Bernacki <g...@semihalf.com> > Sent: Friday, July 9, 2021 5:55 PM > To: Gao, Zhichao <zhichao....@intel.com> > Cc: devel@edk2.groups.io; l...@nuviainc.com; ardb+tianoc...@kernel.org; > samer.el-haj-mahm...@arm.com; sunny.w...@arm.com; > m...@semihalf.com; upstr...@semihalf.com; p...@akeo.ie; Wang, Jian J > <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Bi, Dandan > <dandan...@intel.com>; Dong, Eric <eric.d...@intel.com> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Add > BootDiscoveryPolicyUiLib. > > Hi Zhichao, > > Setting HII-type PCD causes variable initialization, so if > GetVariable() fails due to variable not being found, it will be > initialized by PcdSet32S() function. > thanks, > greg > > czw., 8 lip 2021 o 10:08 Gao, Zhichao <zhichao....@intel.com> napisaĆ(a): > > > > See below comments. > > > > > -----Original Message----- > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > > > Grzegorz Bernacki > > > Sent: Tuesday, July 6, 2021 6:45 PM > > > To: devel@edk2.groups.io > > > Cc: l...@nuviainc.com; ardb+tianoc...@kernel.org; Samer.El-Haj- > > > mahm...@arm.com; sunny.w...@arm.com; m...@semihalf.com; > > > upstr...@semihalf.com; p...@akeo.ie; Wang, Jian J > > > <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Bi, Dandan > > > <dandan...@intel.com>; Dong, Eric <eric.d...@intel.com>; Grzegorz > > > Bernacki <g...@semihalf.com>; Sunny Wang <sunny.w...@arm.com> > > > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Add > > > BootDiscoveryPolicyUiLib. > > > > > > This library extends Boot Maintenance Menu and allows to select Boot > > > Discovery Policy. When choice is made BootDiscoveryPolicy variable is set. > > > Platform code can use this variable to decide which class of device shall > > > be > > > connected. > > > > > > Signed-off-by: Grzegorz Bernacki <g...@semihalf.com> > > > Reviewed-by: Sunny Wang <sunny.w...@arm.com> > > > --- > > > MdeModulePkg/MdeModulePkg.dec > > > | 6 + > > > > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib. > > > inf | 52 +++++++ > > > MdeModulePkg/Include/Guid/BootDiscoveryPolicy.h > > > | > 22 > > > +++ > > > > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib. > > > c | 160 ++++++++++++++++++++ > > > > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib. > > > uni | 16 ++ > > > > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib > > > Strings.uni | 29 ++++ > > > > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib > > > Vfr.Vfr | 44 ++++++ > > > 7 files changed, 329 insertions(+) > > > create mode 100644 > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib. > > > inf > > > create mode 100644 MdeModulePkg/Include/Guid/BootDiscoveryPolicy.h > > > create mode 100644 > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib. > > > c > > > create mode 100644 > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib. > > > uni > > > create mode 100644 > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib > > > Strings.uni > > > create mode 100644 > > > MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLib > > > Vfr.Vfr > > > > > > diff --git a/MdeModulePkg/MdeModulePkg.dec > > > b/MdeModulePkg/MdeModulePkg.dec index ad84421cf3..4e1c291768 > > > 100644 > > > --- a/MdeModulePkg/MdeModulePkg.dec > > > +++ b/MdeModulePkg/MdeModulePkg.dec > > > @@ -425,6 +425,9 @@ > > > ## Include/UniversalPayload/SerialPortInfo.h > > > gUniversalPayloadSerialPortInfoGuid = { 0xaa7e190d, 0xbe21, 0x4409, > > > { 0x8e, 0x67, 0xa2, 0xcd, 0xf, 0x61, 0xe1, 0x70 } } > > > > > > + ## GUID used for Boot Discovery Policy FormSet guid and related > > > variables. > > > + gBootDiscoveryPolicyMgrFormsetGuid = { 0x5b6f7107, 0xbb3c, 0x4660, { > > > + 0x92, 0xcd, 0x54, 0x26, 0x90, 0x28, 0x0b, 0xbd } } > > > + > > > [Ppis] > > > ## Include/Ppi/AtaController.h > > > gPeiAtaControllerPpiGuid = { 0xa45e60d1, 0xc719, 0x44aa, { 0xb0, > > > 0x7a, > > > 0xaa, 0x77, 0x7f, 0x85, 0x90, 0x6d }} > > > @@ -1600,6 +1603,9 @@ > > > # @Prompt Console Output Row of Text Setup > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSetupConOutRow|25|UINT32|0x40 > > > 00000e > > > > > > + ## Specify the Boot Discovery Policy settings > > > + > > > gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy|2|UINT32|0x4 > > > 0000 > > > + 00f > > > + > > > [PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64] > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20|UI > > > NT32|0x0001004c > > > > > > diff --git > > > > a/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > b.inf > > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > b.inf > > > new file mode 100644 > > > index 0000000000..1fb4d43caa > > > --- /dev/null > > > +++ > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyU > > > +++ iLib.inf > > > @@ -0,0 +1,52 @@ > > > +## @file > > > +# Library for BDS phase to use Boot Discovery Policy # # Copyright > > > +(c) 2021, ARM Ltd. All rights reserved.<BR> # Copyright (c) 2021, > > > +Semihalf All rights reserved.<BR> # SPDX-License-Identifier: > > > +BSD-2-Clause-Patent # ## > > > + > > > +[Defines] > > > + INF_VERSION = 0x00010005 > > > + BASE_NAME = BootDiscoveryPolicyUiLib > > > + MODULE_UNI_FILE = BootDiscoveryPolicyUiLib.uni > > > + FILE_GUID = BE73105A-B13D-4B57-A41A-463DBD15FE10 > > > + MODULE_TYPE = DXE_DRIVER > > > + VERSION_STRING = 1.0 > > > + LIBRARY_CLASS = NULL|DXE_DRIVER UEFI_APPLICATION > > > + CONSTRUCTOR = BootDiscoveryPolicyUiLibConstructor > > > + DESTRUCTOR = BootDiscoveryPolicyUiLibDestructor > > > +# > > > +# The following information is for reference only and not required by the > > > build tools. > > > +# > > > +# VALID_ARCHITECTURES = IA32 X64 AARCH64 > > > +# > > > + > > > +[Sources] > > > + BootDiscoveryPolicyUiLib.c > > > + BootDiscoveryPolicyUiLibStrings.uni > > > + BootDiscoveryPolicyUiLibVfr.Vfr > > > + > > > +[Packages] > > > + MdePkg/MdePkg.dec > > > + MdeModulePkg/MdeModulePkg.dec > > > + > > > +[LibraryClasses] > > > + DevicePathLib > > > + BaseLib > > > + UefiRuntimeServicesTableLib > > > + UefiBootServicesTableLib > > > + DebugLib > > > + HiiLib > > > + UefiLib > > > + BaseMemoryLib > > > + > > > +[Guids] > > > + gBootDiscoveryPolicyMgrFormsetGuid > > > + > > > +[Pcd] > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy ## > > > PRODUCES > > > + > > > +[Depex] > > > + gEfiHiiDatabaseProtocolGuid AND gPcdProtocolGuid > > > diff --git a/MdeModulePkg/Include/Guid/BootDiscoveryPolicy.h > > > b/MdeModulePkg/Include/Guid/BootDiscoveryPolicy.h > > > new file mode 100644 > > > index 0000000000..8eb0968a16 > > > --- /dev/null > > > +++ b/MdeModulePkg/Include/Guid/BootDiscoveryPolicy.h > > > @@ -0,0 +1,22 @@ > > > +/** @file > > > + Definition for structure & defines exported by Boot Discovery Policy > > > +UI > > > + > > > + Copyright (c) 2021, ARM Ltd. All rights reserved.<BR> Copyright (c) > > > + 2021, Semihalf All rights reserved.<BR> > > > + > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > + > > > +**/ > > > + > > > +#ifndef _BOOT_DISCOVERY_POLICY_UI_LIB_H_ #define > > > +_BOOT_DISCOVERY_POLICY_UI_LIB_H_ > > > + > > > +#define BDP_CONNECT_MINIMAL 0 /* Do not connect any additional > > > devices */ > > > +#define BDP_CONNECT_NET 1 > > > +#define BDP_CONNECT_ALL 2 > > > + > > > +#define BOOT_DISCOVERY_POLICY_MGR_FORMSET_GUID { 0x5b6f7107, > > > 0xbb3c, > > > +0x4660, { 0x92, 0xcd, 0x54, 0x26, 0x90, 0x28, 0x0b, 0xbd } } > > > + > > > +#define BOOT_DISCOVERY_POLICY_VAR L"BootDiscoveryPolicy" > > > + > > > +#endif > > > diff --git > > > > a/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > b.c > > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > b.c > > > new file mode 100644 > > > index 0000000000..6814d0bb8f > > > --- /dev/null > > > +++ > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyU > > > +++ iLib.c > > > @@ -0,0 +1,160 @@ > > > +/** @file > > > + Boot Discovery Policy UI for Boot Maintenance menu. > > > + > > > + Copyright (c) 2021, ARM Ltd. All rights reserved.<BR> Copyright (c) > > > + 2021, Semihalf All rights reserved.<BR> > > > + > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > + > > > +**/ > > > + > > > +#include <Guid/BootDiscoveryPolicy.h> > > > +#include <Library/UefiDriverEntryPoint.h> #include > > > +<Library/UefiBootServicesTableLib.h> > > > +#include <Library/UefiRuntimeServicesTableLib.h> > > > +#include <Library/BaseLib.h> > > > +#include <Library/DevicePathLib.h> > > > +#include <Library/DebugLib.h> > > > +#include <Library/HiiLib.h> > > > +#include <Library/UefiLib.h> > > > +#include <Library/BaseMemoryLib.h> > > > +#include <Include/Library/PcdLib.h> > > > + > > > +/// > > > +/// HII specific Vendor Device Path definition. > > > +/// > > > +typedef struct { > > > + VENDOR_DEVICE_PATH VendorDevicePath; > > > + EFI_DEVICE_PATH_PROTOCOL End; > > > +} HII_VENDOR_DEVICE_PATH; > > > + > > > +extern unsigned char BootDiscoveryPolicyUiLibVfrBin[]; > > > + > > > +EFI_HII_HANDLE mBPHiiHandle = NULL; > > > +EFI_HANDLE mBPDriverHandle = NULL; > > > + > > > +STATIC HII_VENDOR_DEVICE_PATH mVendorDevicePath = { > > > + { > > > + { > > > + HARDWARE_DEVICE_PATH, > > > + HW_VENDOR_DP, > > > + { > > > + (UINT8)(sizeof (VENDOR_DEVICE_PATH)), > > > + (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8) > > > + } > > > + }, > > > + BOOT_DISCOVERY_POLICY_MGR_FORMSET_GUID > > > + }, > > > + { > > > + END_DEVICE_PATH_TYPE, > > > + END_ENTIRE_DEVICE_PATH_SUBTYPE, > > > + { > > > + (UINT8)(END_DEVICE_PATH_LENGTH), > > > + (UINT8)((END_DEVICE_PATH_LENGTH) >> 8) > > > + } > > > + } > > > +}; > > > + > > > +/** > > > + > > > + Initialize Boot Maintenance Menu library. > > > + > > > + @param ImageHandle The image handle. > > > + @param SystemTable The system table. > > > + > > > + @retval EFI_SUCCESS Install Boot manager menu success. > > > + @retval Other Return error status.gBPDisplayLibGuid > > > + > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +BootDiscoveryPolicyUiLibConstructor ( > > > + IN EFI_HANDLE ImageHandle, > > > + IN EFI_SYSTEM_TABLE *SystemTable > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + UINTN Size; > > > + UINT32 BootDiscoveryPolicy; > > > + > > > + Size = sizeof (UINT32); > > > + Status = gRT->GetVariable ( > > > + BOOT_DISCOVERY_POLICY_VAR, > > > + &gBootDiscoveryPolicyMgrFormsetGuid, > > > + NULL, > > > + &Size, > > > + &BootDiscoveryPolicy > > > + ); > > > + if (EFI_ERROR (Status)) { > > > + Status = PcdSet32S (PcdBootDiscoveryPolicy, PcdGet32 > > > (PcdBootDiscoveryPolicy)); > > > + ASSERT_EFI_ERROR (Status); > > > + } > > > > I don't understand the above check. Seems the value of the variable is not > > used > and the Pcd value is not changed. > > > > Thanks, > > Zhichao > > > > > + > > > + Status = gBS->InstallMultipleProtocolInterfaces ( > > > + &mBPDriverHandle, > > > + &gEfiDevicePathProtocolGuid, > > > + &mVendorDevicePath, > > > + NULL > > > + ); > > > + if (EFI_ERROR (Status)) { > > > + return Status; > > > + } > > > + > > > + // > > > + // Publish our HII data > > > + // > > > + mBPHiiHandle = HiiAddPackages ( > > > + &gBootDiscoveryPolicyMgrFormsetGuid, > > > + mBPDriverHandle, > > > + BootDiscoveryPolicyUiLibVfrBin, > > > + BootDiscoveryPolicyUiLibStrings, > > > + NULL > > > + ); > > > + if (mBPHiiHandle == NULL) { > > > + gBS->UninstallMultipleProtocolInterfaces ( > > > + mBPDriverHandle, > > > + &gEfiDevicePathProtocolGuid, > > > + &mVendorDevicePath, > > > + NULL > > > + ); > > > + > > > + return EFI_OUT_OF_RESOURCES; > > > + } > > > + > > > + return EFI_SUCCESS; > > > +} > > > + > > > +/** > > > + Destructor of Boot Maintenance menu library. > > > + > > > + @param ImageHandle The firmware allocated handle for the EFI image. > > > + @param SystemTable A pointer to the EFI System Table. > > > + > > > + @retval EFI_SUCCESS The destructor completed successfully. > > > + @retval Other value The destructor did not complete successfully. > > > + > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +BootDiscoveryPolicyUiLibDestructor ( > > > + IN EFI_HANDLE ImageHandle, > > > + IN EFI_SYSTEM_TABLE *SystemTable > > > + ) > > > +{ > > > + > > > + if (mBPDriverHandle != NULL) { > > > + gBS->UninstallProtocolInterface ( > > > + mBPDriverHandle, > > > + &gEfiDevicePathProtocolGuid, > > > + &mVendorDevicePath > > > + ); > > > + mBPDriverHandle = NULL; > > > + } > > > + > > > + if (mBPHiiHandle != NULL) { > > > + HiiRemovePackages (mBPHiiHandle); > > > + mBPHiiHandle = NULL; > > > + } > > > + > > > + return EFI_SUCCESS; > > > +} > > > diff --git > > > > a/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > b.uni > > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > b.uni > > > new file mode 100644 > > > index 0000000000..89231bc2d7 > > > --- /dev/null > > > +++ > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyU > > > +++ iLib.uni > > > @@ -0,0 +1,16 @@ > > > +// /** @file > > > +// Boot Discovery Policy UI module. > > > +// > > > +// Copyright (c) 2021, ARM Ltd. All rights reserved.<BR> // Copyright > > > +(c) 2021, Semihalf All rights reserved.<BR> // // > > > +SPDX-License-Identifier: BSD-2-Clause-Patent // // **/ > > > + > > > + > > > +#string STR_MODULE_ABSTRACT > > > +#language en-US "Boot Discovery Policy UI module." > > > + > > > +#string STR_MODULE_DESCRIPTION > > > +#language en-US "Boot Discovery Policy UI module." > > > diff --git > > > > a/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > bStrings.uni > > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > bStrings.uni > > > new file mode 100644 > > > index 0000000000..736011c9bb > > > --- /dev/null > > > +++ > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyU > > > +++ iLibStrings.uni > > > @@ -0,0 +1,29 @@ > > > +// *++ > > > +// > > > +// Copyright (c) 2021, ARM Ltd. All rights reserved.<BR> // Copyright > > > +(c) 2021, Semihalf All rights reserved.<BR> // > > > +SPDX-License-Identifier: BSD-2-Clause-Patent // // Module Name: > > > +// > > > +// BootDiscoveryPolicyUiLibStrings.uni > > > +// > > > +// Abstract: > > > +// > > > +// String definitions for Boot Discovery Policy UI. > > > +// > > > +// --*/ > > > + > > > +/=# > > > + > > > + > > > +#langdef en-US "English" > > > + > > > +#string STR_FORM_BDP_MAIN_TITLE #language en-US "Boot > Discovery > > > Policy" > > > + > > > +#string STR_FORM_BDP_CONN_MIN #language en-US "Minimal" > > > + > > > +#string STR_FORM_BDP_CONN_NET #language en-US "Connect > > > Network Devices" > > > + > > > +#string STR_FORM_BDP_CONN_ALL #language en-US "Connect All > > > Devices" > > > + > > > diff --git > > > > a/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > bVfr.Vfr > > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyUiLi > > > bVfr.Vfr > > > new file mode 100644 > > > index 0000000000..0de87ec34f > > > --- /dev/null > > > +++ > > > b/MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolicyU > > > +++ iLibVfr.Vfr > > > @@ -0,0 +1,44 @@ > > > +///** @file > > > +// > > > +// Formset for Boot Discovery Policy UI // // Copyright (c) 2021, ARM > > > +Ltd. All rights reserved.<BR> // Copyright (c) 2021, Semihalf All > > > +rights reserved.<BR> // // SPDX-License-Identifier: > > > +BSD-2-Clause-Patent // //**/ > > > + > > > +#include <Uefi/UefiMultiPhase.h> > > > +#include "Guid/BootDiscoveryPolicy.h" > > > +#include <Guid/HiiBootMaintenanceFormset.h> > > > + > > > +typedef struct { > > > + UINT32 BootDiscoveryPolicy; > > > +} BOOT_DISCOVERY_POLICY_VARSTORE_DATA; > > > + > > > +formset > > > + guid = BOOT_DISCOVERY_POLICY_MGR_FORMSET_GUID, > > > + title = STRING_TOKEN(STR_FORM_BDP_MAIN_TITLE), > > > + help = STRING_TOKEN(STR_FORM_BDP_MAIN_TITLE), > > > + classguid = EFI_IFR_BOOT_MAINTENANCE_GUID, > > > + > > > + efivarstore BOOT_DISCOVERY_POLICY_VARSTORE_DATA, > > > + attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | > > > EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE, > > > + name = BootDiscoveryPolicy, > > > + guid = BOOT_DISCOVERY_POLICY_MGR_FORMSET_GUID; > > > + > > > + form formid = 0x0001, > > > + title = STRING_TOKEN(STR_FORM_BDP_MAIN_TITLE); > > > + > > > + oneof varid = BootDiscoveryPolicy.BootDiscoveryPolicy, > > > + prompt = STRING_TOKEN(STR_FORM_BDP_MAIN_TITLE), > > > + help = STRING_TOKEN(STR_FORM_BDP_MAIN_TITLE), > > > + flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED, > > > + option text = STRING_TOKEN(STR_FORM_BDP_CONN_MIN), value = > > > BDP_CONNECT_MINIMAL, flags = DEFAULT; > > > + option text = STRING_TOKEN(STR_FORM_BDP_CONN_NET), value = > > > BDP_CONNECT_NET, flags = 0; > > > + option text = STRING_TOKEN(STR_FORM_BDP_CONN_ALL), value = > > > + BDP_CONNECT_ALL, flags = 0; endoneof; > > > + > > > + endform; > > > +endformset; > > > -- > > > 2.25.1 > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78005): https://edk2.groups.io/g/devel/message/78005 Mute This Topic: https://groups.io/mt/84017522/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-