[edk2-devel] [PATCH v5 0/2] SecurityPkg/OpalPasswordDxe: Update according to UEFI spec

[edk2-devel] [PATCH v5 0/2] SecurityPkg/OpalPasswordDxe: Update according to UEFI spec

[Cindy Kuo]

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, which is not allowed in ACTION_FORM_OPEN callback function.

To meet UEFI 2.9 spec, a latency issue will be observed if the browser callback action changes from ACTION_FORM_OPEN to ACTION_RETRIEVE.
The NVNE disks will not be displayed when the user enters the formset at the first time. Revisit the formset can see the update.
So need to force reparsing the IFR binary when RETRIEVE.

v2:
Format code with Uncrustify.

v3:
Code refine based on comments from Dandan and Tina.

v4:
Split solution into two patches as different purpose.

v5:
Update commit message.

Cindy Kuo (2):
  SecurityPkg/OpalPasswordDxe: Change callback action to meet UEFI spec
  SecurityPkg/OpalPasswordDxe: Force reparsing IFR binary when RETRIEVE

 .../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(-)

-- 
2.44.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117846): https://edk2.groups.io/g/devel/message/117846
Mute This Topic: https://groups.io/mt/105551557/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v5 1/2] SecurityPkg/OpalPasswordDxe: Change callback action to meet UEFI spec

[Cindy Kuo]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4735

Should not call HiiGetBrowserData() and HiiSetBrowserData() in
ACTION_FORM_OPEN callback function.
Those APIs are called within OpalHiiSetBrowserData/OpalHiiGetBrowserData
which have been used by OpalHii.c.

Change callback action from FORM_OPEN to RETRIEVE to meet UEFI spec.

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>
Reviewed-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Tina Chen <tina.chen@intel.com>
---
 SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
index 8035f44ebe..56ada1a9f3 100644
--- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
+++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
@@ -632,7 +632,7 @@ DriverCallback (
   HiiKey.Raw = QuestionId;
   HiiKeyId   = (UINT8)HiiKey.KeyBits.Id;
 
-  if (Action == EFI_BROWSER_ACTION_FORM_OPEN) {
+  if (Action == EFI_BROWSER_ACTION_RETRIEVE) {
     switch (HiiKeyId) {
       case HII_KEY_ID_VAR_SUPPORTED_DISKS:
         DEBUG ((DEBUG_INFO, "HII_KEY_ID_VAR_SUPPORTED_DISKS\n"));
-- 
2.44.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117847): https://edk2.groups.io/g/devel/message/117847
Mute This Topic: https://groups.io/mt/105551558/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v5 2/2] SecurityPkg/OpalPasswordDxe: Force reparsing IFR binary when RETRIEVE

[Cindy Kuo]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4735

When change callback action from FORM_OPEN to RETRIEVE, it is observed
that NVNE disks will not be displayed when the user enters the formset
at the first time. Revisit the formset can see the update.

1. Add HiiUpdateForm() to force reparsing the IFR binary when RETRIEVE.
2. Create dummy label with suppressif statement in VFR for form update
usage.

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>
Reviewed-by: Dandan Bi <dandan.bi@intel.com>
Reviewed-by: Tina Chen <tina.chen@intel.com>
---
 .../Tcg/Opal/OpalPassword/OpalDriver.h        |  1 +
 SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c   | 82 ++++++++++++++++---
 .../Tcg/Opal/OpalPassword/OpalHiiFormValues.h |  6 ++
 .../Tcg/Opal/OpalPassword/OpalPasswordDxe.inf |  1 +
 .../Opal/OpalPassword/OpalPasswordForm.vfr    |  8 +-
 5 files changed, 86 insertions(+), 12 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 56ada1a9f3..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;
@@ -633,14 +639,68 @@ DriverCallback (
   HiiKeyId   = (UINT8)HiiKey.KeyBits.Id;
 
   if (Action == EFI_BROWSER_ACTION_RETRIEVE) {
-    switch (HiiKeyId) {
-      case HII_KEY_ID_VAR_SUPPORTED_DISKS:
-        DEBUG ((DEBUG_INFO, "HII_KEY_ID_VAR_SUPPORTED_DISKS\n"));
-        return HiiPopulateMainMenuForm ();
+    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 (#117848): https://edk2.groups.io/g/devel/message/117848
Mute This Topic: https://groups.io/mt/105551559/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v5 0/2] SecurityPkg/OpalPasswordDxe: Update according to UEFI spec

[Yao, Jiewen]

Merged https://github.com/tianocore/edk2/pull/5563

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Cindy Kuo
> Sent: Tuesday, April 16, 2024 1:03 PM
> To: devel@edk2.groups.io
> Cc: Kuo, CindyX <cindyx.kuo@intel.com>
> Subject: [edk2-devel] [PATCH v5 0/2] SecurityPkg/OpalPasswordDxe: Update
> according to UEFI spec
> 
> 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, which is not allowed
> in ACTION_FORM_OPEN callback function.
> 
> To meet UEFI 2.9 spec, a latency issue will be observed if the browser callback
> action changes from ACTION_FORM_OPEN to ACTION_RETRIEVE.
> The NVNE disks will not be displayed when the user enters the formset at the first
> time. Revisit the formset can see the update.
> So need to force reparsing the IFR binary when RETRIEVE.
> 
> v2:
> Format code with Uncrustify.
> 
> v3:
> Code refine based on comments from Dandan and Tina.
> 
> v4:
> Split solution into two patches as different purpose.
> 
> v5:
> Update commit message.
> 
> Cindy Kuo (2):
>   SecurityPkg/OpalPasswordDxe: Change callback action to meet UEFI spec
>   SecurityPkg/OpalPasswordDxe: Force reparsing IFR binary when RETRIEVE
> 
>  .../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(-)
> 
> --
> 2.44.0.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117869): https://edk2.groups.io/g/devel/message/117869
Mute This Topic: https://groups.io/mt/105551557/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-