* [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
@ 2024-04-11 3:10 Cindy Kuo
2024-04-11 11:15 ` Dandan Bi
0 siblings, 1 reply; 6+ messages in thread
From: Cindy Kuo @ 2024-04-11 3:10 UTC (permalink / raw)
To: devel
Cc: Cindy Kuo, Jiewen Yao, Rahul Kumar, Dandan Bi, Ming Tan,
Arthur Chen, Xiao X Chen, Tina Chen
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 (#117608): https://edk2.groups.io/g/devel/message/117608
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
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
0 siblings, 1 reply; 6+ messages in thread
From: Dandan Bi @ 2024-04-11 11:15 UTC (permalink / raw)
To: Kuo, CindyX, devel@edk2.groups.io
Cc: Yao, Jiewen, Kumar, Rahul R, Tan, Ming, Chen, Arthur G,
Chen, Xiao X, Chen, Tina
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 (#117632): https://edk2.groups.io/g/devel/message/117632
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
2024-04-11 11:15 ` Dandan Bi
@ 2024-04-11 15:45 ` Yao, Jiewen
2024-04-12 3:24 ` Tina Chen
0 siblings, 1 reply; 6+ messages in thread
From: Yao, Jiewen @ 2024-04-11 15:45 UTC (permalink / raw)
To: Bi, Dandan, Kuo, CindyX, devel@edk2.groups.io
Cc: Kumar, Rahul R, Tan, Ming, Chen, Arthur G, Chen, Xiao X,
Chen, Tina
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
2024-04-11 15:45 ` Yao, Jiewen
@ 2024-04-12 3:24 ` Tina Chen
2024-04-12 4:05 ` Yao, Jiewen
0 siblings, 1 reply; 6+ messages in thread
From: Tina Chen @ 2024-04-12 3:24 UTC (permalink / raw)
To: Yao, Jiewen, Bi, Dandan, Kuo, CindyX, devel@edk2.groups.io
Cc: Kumar, Rahul R, Tan, Ming, Chen, Arthur G, Chen, Xiao X
Hi JieWen,
For opalHii current design, it will display all NVME disks when the user enters TCG Drive Management dynamically.
Also, the related disk info form will be created along with the disks.
These actions will call get/set browser to refresh the display.
To meet UEFI 2.9 spec, a latency issue will be observed if the browser action changes from ACTION_FORM_OPEN to ACTION_RETRIEVE due to the current Hii browser design flow.
The NVNE disks will not be able to display when the user enters the formset. (Revisit the formset can see the update.)
After discussing with Dandan, came up with a solution to force reparsing the IFR binary when RETRIEVE.
That's why it needs to have additional changes besides changing the execute action only.
Thanks.
Sincerely,
Tina
-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Thursday, April 11, 2024 23:45
To: Bi, Dandan <dandan.bi@intel.com>; Kuo, CindyX <cindyx.kuo@intel.com>; 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: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
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
--
Tina Chen
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117639): https://edk2.groups.io/g/devel/message/117639
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
2024-04-12 3:24 ` Tina Chen
@ 2024-04-12 4:05 ` Yao, Jiewen
2024-04-12 7:05 ` Cindy Kuo
0 siblings, 1 reply; 6+ messages in thread
From: Yao, Jiewen @ 2024-04-12 4:05 UTC (permalink / raw)
To: Chen, Tina, Bi, Dandan, Kuo, CindyX, devel@edk2.groups.io
Cc: Kumar, Rahul R, Tan, Ming, Chen, Arthur G, Chen, Xiao X
Thanks to explain the background to me. I appreciate that.
Also I trust Dandan's judgement as the UI expert.
But my question remains: Are 2 and 3 related to UEFI spec update? IMHO, they are NOT required if we just want to do update for UEFI spec.
If it is such case, please file a new issue, or split them into different patch.
In each patch, please explain as clear as possible, on why it is needed.
That will help reviewer or maintainer to have better understanding.
Last but not least, please describe what test you have done for the patch.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: Chen, Tina <tina.chen@intel.com>
> Sent: Friday, April 12, 2024 11:25 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
> Kuo, CindyX <cindyx.kuo@intel.com>; 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>
> Subject: RE: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to
> UEFI spec
>
> Hi JieWen,
>
> For opalHii current design, it will display all NVME disks when the user enters TCG
> Drive Management dynamically.
> Also, the related disk info form will be created along with the disks.
> These actions will call get/set browser to refresh the display.
> To meet UEFI 2.9 spec, a latency issue will be observed if the browser action
> changes from ACTION_FORM_OPEN to ACTION_RETRIEVE due to the current Hii
> browser design flow.
> The NVNE disks will not be able to display when the user enters the formset.
> (Revisit the formset can see the update.)
> After discussing with Dandan, came up with a solution to force reparsing the IFR
> binary when RETRIEVE.
> That's why it needs to have additional changes besides changing the execute
> action only.
> Thanks.
>
> Sincerely,
> Tina
>
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Thursday, April 11, 2024 23:45
> To: Bi, Dandan <dandan.bi@intel.com>; Kuo, CindyX <cindyx.kuo@intel.com>;
> 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: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to
> UEFI spec
>
> 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 (#117640): https://edk2.groups.io/g/devel/message/117640
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
2024-04-12 4:05 ` Yao, Jiewen
@ 2024-04-12 7:05 ` Cindy Kuo
0 siblings, 0 replies; 6+ messages in thread
From: Cindy Kuo @ 2024-04-12 7:05 UTC (permalink / raw)
To: Yao, Jiewen, Chen, Tina, Bi, Dandan, devel@edk2.groups.io
Cc: Kumar, Rahul R, Tan, Ming, Chen, Arthur G, Chen, Xiao X
Hi Jiewen,
As you comment, I'll split them into two patch and upload later.
And here is the test I've done for the patch.
1. Enter and exit Opal setup main menu and sub-menu disk info page, and make sure no latency issue is observed.
2. Set password for Opal device and check boot successfully when unlock device with correct password.
3. Set password for Opal device and check system would force to shutdown when unlock device with incorrect password and retry count exceeds the limit.
Thanks,
Cindy Kuo
-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Friday, April 12, 2024 12:06 PM
To: Chen, Tina <tina.chen@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kuo, CindyX <cindyx.kuo@intel.com>; 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>
Subject: RE: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI according to UEFI spec
Thanks to explain the background to me. I appreciate that.
Also I trust Dandan's judgement as the UI expert.
But my question remains: Are 2 and 3 related to UEFI spec update? IMHO, they are NOT required if we just want to do update for UEFI spec.
If it is such case, please file a new issue, or split them into different patch.
In each patch, please explain as clear as possible, on why it is needed.
That will help reviewer or maintainer to have better understanding.
Last but not least, please describe what test you have done for the patch.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: Chen, Tina <tina.chen@intel.com>
> Sent: Friday, April 12, 2024 11:25 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Kuo, CindyX <cindyx.kuo@intel.com>;
> 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>
> Subject: RE: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI
> according to UEFI spec
>
> Hi JieWen,
>
> For opalHii current design, it will display all NVME disks when the
> user enters TCG Drive Management dynamically.
> Also, the related disk info form will be created along with the disks.
> These actions will call get/set browser to refresh the display.
> To meet UEFI 2.9 spec, a latency issue will be observed if the browser
> action changes from ACTION_FORM_OPEN to ACTION_RETRIEVE due to the
> current Hii browser design flow.
> The NVNE disks will not be able to display when the user enters the formset.
> (Revisit the formset can see the update.) After discussing with
> Dandan, came up with a solution to force reparsing the IFR binary when
> RETRIEVE.
> That's why it needs to have additional changes besides changing the
> execute action only.
> Thanks.
>
> Sincerely,
> Tina
>
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Thursday, April 11, 2024 23:45
> To: Bi, Dandan <dandan.bi@intel.com>; Kuo, CindyX
> <cindyx.kuo@intel.com>; 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: [PATCH v3] SecurityPkg/OpalPasswordDxe: Update UI
> according to UEFI spec
>
> 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 (#117643): https://edk2.groups.io/g/devel/message/117643
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-12 7:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2024-04-12 3:24 ` Tina Chen
2024-04-12 4:05 ` Yao, Jiewen
2024-04-12 7:05 ` Cindy Kuo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox