Hi It seems this patch adds more change than just "update UI according to UEFI spec".
Please help me understand why we need below 2 and 3. Are you required for UEFI spec update? > 2. Create dummy label with suppressif statement in VFR for form update usage. > 3. Add HiiUpdateForm() to force reparsing the IFR binary. Thank you Yao, Jiewen > -----Original Message----- > From: Bi, Dandan <dandan...@intel.com> > Sent: Thursday, April 11, 2024 7:15 PM > To: Kuo, CindyX <cindyx....@intel.com>; devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen....@intel.com>; Kumar, Rahul R > <rahul.r.ku...@intel.com>; Tan, Ming <ming....@intel.com>; Chen, Arthur G > <arthur.g.c...@intel.com>; Chen, Xiao X <xiao.x.c...@intel.com>; Chen, Tina > <tina.c...@intel.com> > Subject: RE: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to > UEFI spec > > Reviewed-by: Dandan Bi <dandan...@intel.com> > > > Thanks, > Dandan > -----Original Message----- > From: Kuo, CindyX <cindyx....@intel.com> > Sent: Thursday, April 11, 2024 11:11 AM > To: devel@edk2.groups.io > Cc: Kuo, CindyX <cindyx....@intel.com>; Yao, Jiewen <jiewen....@intel.com>; > Kumar, Rahul R <rahul.r.ku...@intel.com>; Bi, Dandan <dandan...@intel.com>; > Tan, Ming <ming....@intel.com>; Chen, Arthur G <arthur.g.c...@intel.com>; > Chen, Xiao X <xiao.x.c...@intel.com>; Chen, Tina <tina.c...@intel.com> > Subject: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI > spec > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4735 > > Should not call HiiGetBrowserData() and HiiSetBrowserData() in FORM_OPEN call > back function. > Those APIs are called within OpalHiiSetBrowserData/OpalHiiGetBrowserData > which have been used by OpalHii.c. > > 1. Change callback action from FORM_OPEN to RETRIEVE. > 2. Create dummy label with suppressif statement in VFR for form update usage. > 3. Add HiiUpdateForm() to force reparsing the IFR binary. > > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Rahul Kumar <rahul1.ku...@intel.com> > Cc: Dandan Bi <dandan...@intel.com> > Cc: Ming Tan <ming....@intel.com> > Cc: Arthur Chen <arthur.g.c...@intel.com> > Cc: Xiao X Chen <xiao.x.c...@intel.com> > Cc: Tina Chen <tina.c...@intel.com> > Signed-off-by: CindyX Kuo <cindyx....@intel.com> > --- > .../Tcg/Opal/OpalPassword/OpalDriver.h | 1 + > SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c | 84 ++++++++++++++++--- > .../Tcg/Opal/OpalPassword/OpalHiiFormValues.h | 6 > ++ .../Tcg/Opal/OpalPassword/OpalPasswordDxe.inf | 1 + > .../Opal/OpalPassword/OpalPasswordForm.vfr | 8 +- > 5 files changed, 87 insertions(+), 13 deletions(-) > > diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h > b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h > index 2089bd81b6..1a4671c602 100644 > --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h > +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h > @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include <Guid/EventGroup.h> > #include <Guid/S3StorageDeviceInitList.h> > +#include <Guid/MdeModuleHii.h> > > #include <Library/UefiLib.h> > #include <Library/UefiBootServicesTableLib.h> > diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c > b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c > index 8035f44ebe..47af4fee40 100644 > --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c > +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c > @@ -40,6 +40,7 @@ EFI_HII_HANDLE gHiiPackageListHandle = NULL; // > const EFI_GUID gHiiPackageListGuid = PACKAGE_LIST_GUID; > const EFI_GUID gHiiSetupVariableGuid = SETUP_VARIABLE_GUID; > +const EFI_GUID gOpalSetupFormSetGuid = SETUP_FORMSET_GUID; > > // > // Structure that contains state of the HII @@ -611,10 +612,15 @@ > DriverCallback ( > EFI_BROWSER_ACTION_REQUEST *ActionRequest > ) > { > - HII_KEY HiiKey; > - UINT8 HiiKeyId; > - UINT32 PpRequest; > - OPAL_DISK *OpalDisk; > + HII_KEY HiiKey; > + UINT8 HiiKeyId; > + UINT32 PpRequest; > + OPAL_DISK *OpalDisk; > + EFI_STATUS Status; > + VOID *StartOpCodeHandle; > + VOID *EndOpCodeHandle; > + EFI_IFR_GUID_LABEL *StartLabel; > + EFI_IFR_GUID_LABEL *EndLabel; > > if (ActionRequest != NULL) { > *ActionRequest = EFI_BROWSER_ACTION_REQUEST_NONE; @@ -632,15 > +638,69 @@ DriverCallback ( > HiiKey.Raw = QuestionId; > HiiKeyId = (UINT8)HiiKey.KeyBits.Id; > > - if (Action == EFI_BROWSER_ACTION_FORM_OPEN) { > - switch (HiiKeyId) { > - case HII_KEY_ID_VAR_SUPPORTED_DISKS: > - DEBUG ((DEBUG_INFO, "HII_KEY_ID_VAR_SUPPORTED_DISKS\n")); > - return HiiPopulateMainMenuForm (); > + if (Action == EFI_BROWSER_ACTION_RETRIEVE) { > + if ((HiiKeyId == HII_KEY_ID_VAR_SUPPORTED_DISKS) || (HiiKeyId == > HII_KEY_ID_VAR_SELECTED_DISK_AVAILABLE_ACTIONS)) { > + // > + // Allocate space for creation of UpdateData Buffer > + // > + StartOpCodeHandle = HiiAllocateOpCodeHandle (); > + if (StartOpCodeHandle == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + EndOpCodeHandle = HiiAllocateOpCodeHandle (); > + if (EndOpCodeHandle == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + // > + // Create Hii Extend Label OpCode as the start opcode > + // > + StartLabel = (EFI_IFR_GUID_LABEL *)HiiCreateGuidOpCode > (StartOpCodeHandle, &gEfiIfrTianoGuid, NULL, sizeof (EFI_IFR_GUID_LABEL)); > + StartLabel->ExtendOpCode = EFI_IFR_EXTEND_OP_LABEL; > + > + // > + // Create Hii Extend Label OpCode as the end opcode > + // > + EndLabel = (EFI_IFR_GUID_LABEL *)HiiCreateGuidOpCode > (EndOpCodeHandle, &gEfiIfrTianoGuid, NULL, sizeof (EFI_IFR_GUID_LABEL)); > + EndLabel->ExtendOpCode = EFI_IFR_EXTEND_OP_LABEL; > + > + switch (HiiKeyId) { > + case HII_KEY_ID_VAR_SUPPORTED_DISKS: > + DEBUG ((DEBUG_INFO, "HII_KEY_ID_VAR_SUPPORTED_DISKS\n")); > + Status = HiiPopulateMainMenuForm (); > + > + StartLabel->Number = OPAL_MAIN_MENU_LABEL_START; > + EndLabel->Number = OPAL_MAIN_MENU_LABEL_END; > + HiiUpdateForm ( > + gHiiPackageListHandle, > + (EFI_GUID *)&gOpalSetupFormSetGuid, > + FORMID_VALUE_MAIN_MENU, > + StartOpCodeHandle, > + EndOpCodeHandle > + ); > + break; > + > + case HII_KEY_ID_VAR_SELECTED_DISK_AVAILABLE_ACTIONS: > + DEBUG ((DEBUG_INFO, > "HII_KEY_ID_VAR_SELECTED_DISK_AVAILABLE_ACTIONS\n")); > + Status = HiiPopulateDiskInfoForm (); > + > + StartLabel->Number = OPAL_DISK_INFO_LABEL_START; > + EndLabel->Number = OPAL_DISK_INFO_LABEL_END; > + HiiUpdateForm ( > + gHiiPackageListHandle, > + (EFI_GUID *)&gOpalSetupFormSetGuid, > + FORMID_VALUE_DISK_INFO_FORM_MAIN, > + StartOpCodeHandle, > + EndOpCodeHandle > + ); > + break; > + } > + > + HiiFreeOpCodeHandle (StartOpCodeHandle); > + HiiFreeOpCodeHandle (EndOpCodeHandle); > > - case HII_KEY_ID_VAR_SELECTED_DISK_AVAILABLE_ACTIONS: > - DEBUG ((DEBUG_INFO, > "HII_KEY_ID_VAR_SELECTED_DISK_AVAILABLE_ACTIONS\n")); > - return HiiPopulateDiskInfoForm (); > + return Status; > } > } else if (Action == EFI_BROWSER_ACTION_CHANGING) { > switch (HiiKeyId) { > diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHiiFormValues.h > b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHiiFormValues.h > index ab6957fc6f..0e098854ba 100644 > --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHiiFormValues.h > +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHiiFormValues.h > @@ -96,6 +96,12 @@ typedef struct { > > #define HII_KEY(id) HII_KEY_WITH_INDEX(id, 0) > > +/* Label */ > +#define OPAL_MAIN_MENU_LABEL_START 0x6100 > +#define OPAL_MAIN_MENU_LABEL_END 0x6101 > +#define OPAL_DISK_INFO_LABEL_START 0x6200 > +#define OPAL_DISK_INFO_LABEL_END 0x6201 > + > #define PACKAGE_LIST_GUID { 0xf0308176, 0x9058, 0x4153, { 0x93, 0x3d, 0xda, > 0x2f, 0xdc, 0xc8, 0x3e, 0x44 } } > > /* {410483CF-F4F9-4ece-848A-1958FD31CEB7} */ diff --git > a/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordDxe.inf > b/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordDxe.inf > index 87519198c0..89e72a74bc 100644 > --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordDxe.inf > +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordDxe.inf > @@ -69,6 +69,7 @@ > [Guids] > gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event > gS3StorageDeviceInitListGuid ## SOMETIMES_PRODUCES ## > UNDEFINED > + gEfiIfrTianoGuid ## CONSUMES > > [Pcd] > gEfiSecurityPkgTokenSpaceGuid.PcdSkipOpalPasswordPrompt ## CONSUMES > diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordForm.vfr > b/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordForm.vfr > index f0d3e220b2..a1049686ff 100644 > --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordForm.vfr > +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordForm.vfr > @@ -25,8 +25,11 @@ formset > form formid = FORMID_VALUE_MAIN_MENU, > title = STRING_TOKEN(STR_OPAL); > > - //CONFIG_VARIABLE(HII_KEY(HII_KEY_ID_VAR_SUPPORTED_DISKS), > SupportedDisks, 0x0, 0xFFFF); > suppressif TRUE; > + label OPAL_MAIN_MENU_LABEL_START; > + label OPAL_MAIN_MENU_LABEL_END; > + > + //CONFIG_VARIABLE(HII_KEY(HII_KEY_ID_VAR_SUPPORTED_DISKS), > + SupportedDisks, 0x0, 0xFFFF); > numeric > name = SupportedDisks, > varid = OpalHiiConfig.SupportedDisks, > @@ -149,6 +152,9 @@ form formid = > FORMID_VALUE_DISK_INFO_FORM_MAIN, > title = STRING_TOKEN(STR_OPAL); > > suppressif TRUE; > + label OPAL_DISK_INFO_LABEL_START; > + label OPAL_DISK_INFO_LABEL_END; > + > numeric > name = SelectedDiskAvailableActions, > varid = OpalHiiConfig.SelectedDiskAvailableActions, > -- > 2.44.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117634): https://edk2.groups.io/g/devel/message/117634 Mute This Topic: https://groups.io/mt/105456188/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
