From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "Bi, Dandan" <dandan.bi@intel.com>,
"Kuo, CindyX" <cindyx.kuo@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Kumar, Rahul R" <rahul.r.kumar@intel.com>,
"Tan, Ming" <ming.tan@intel.com>,
"Chen, Arthur G" <arthur.g.chen@intel.com>,
"Chen, Xiao X" <xiao.x.chen@intel.com>,
"Chen, Tina" <tina.chen@intel.com>
Subject: Re: [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
Date: Thu, 11 Apr 2024 15:45:01 +0000 [thread overview]
Message-ID: <MW4PR11MB58726F9E58FEC4A1E15790AE8C052@MW4PR11MB5872.namprd11.prod.outlook.com> (raw)
In-Reply-To: <MN6PR11MB824217201820D2A7E8374DE9EA052@MN6PR11MB8242.namprd11.prod.outlook.com>
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.bi@intel.com>
> Sent: Thursday, April 11, 2024 7:15 PM
> To: Kuo, CindyX <cindyx.kuo@intel.com>; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Tan, Ming <ming.tan@intel.com>; Chen, Arthur G
> <arthur.g.chen@intel.com>; Chen, Xiao X <xiao.x.chen@intel.com>; Chen, Tina
> <tina.chen@intel.com>
> Subject: RE: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to
> UEFI spec
>
> Reviewed-by: Dandan Bi <dandan.bi@intel.com>
>
>
> Thanks,
> Dandan
> -----Original Message-----
> From: Kuo, CindyX <cindyx.kuo@intel.com>
> Sent: Thursday, April 11, 2024 11:11 AM
> To: devel@edk2.groups.io
> Cc: Kuo, CindyX <cindyx.kuo@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
> Tan, Ming <ming.tan@intel.com>; Chen, Arthur G <arthur.g.chen@intel.com>;
> Chen, Xiao X <xiao.x.chen@intel.com>; Chen, Tina <tina.chen@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.yao@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Ming Tan <ming.tan@intel.com>
> Cc: Arthur Chen <arthur.g.chen@intel.com>
> Cc: Xiao X Chen <xiao.x.chen@intel.com>
> Cc: Tina Chen <tina.chen@intel.com>
> Signed-off-by: CindyX Kuo <cindyx.kuo@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-04-11 15:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 3:10 [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec Cindy Kuo
2024-04-11 11:15 ` Dandan Bi
2024-04-11 15:45 ` Yao, Jiewen [this message]
2024-04-12 3:24 ` Tina Chen
2024-04-12 4:05 ` Yao, Jiewen
2024-04-12 7:05 ` Cindy Kuo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR11MB58726F9E58FEC4A1E15790AE8C052@MW4PR11MB5872.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox