Re: [PATCH v1 2/3] UserAuthFeaturePkg: Included DXE versions are mutually exclusive

["Benjamin Doron" <benjamin.doron00@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7448 bytes --]

Hi Isaac,

>From memory, the primary difference between the versions of this module is
that v2 uses helper libraries to perform some of the functions in
UserAuthenticationDxePassword.c and the HII helpers in
UserAuthenticationDxe.c. There are other differences, at least including
the handler to challenge for the admin password that should be ported or
implemented for v2, but I think only one DXE module version is necessary to
be in the tree.

We can drop this commit and I can work on improving v2 and deprecating v1 -
as was my plan, because v2 needs to challenge for the password - but not at
the moment. Unless v1's implementation is preferred?

Sorry about the subject prefix. I see that patch 1/3 is in. I think I'll
work on 2/3 later and 3/3 separately.

Best regards,
Benjamin


On Mon, 8 Aug 2022 at 18:51, Oram, Isaac W <isaac.w.oram@intel.com> wrote:

> Benjamin,
>
> Can you update the readme with descriptions of the modules and their
> functionality?
> Can you comment the PcdUseVersion1 declaration in the DEC file?
> If you can think of a more descriptive name than "version 1", that would
> be better.
>
> Note that convention (maybe requirement) is to preface non-edk2 repo
> changes with the mailing list and repo info [edk2-devel][edk2-platforms] so
> that rules can filter more easily.  And multi-commit reviews should have a
> cover-letter enabled as well so you can provide an overview of the patch
> series.
> Here is the command I use: git format-patch -M --thread origin/master
> --cover-letter --subject-prefix="edk2-devel][edk2-platforms][PATCH V1"
>
> Regards,
> Isaac
>
> -----Original Message-----
> From: Benjamin Doron <benjamin.doron00@gmail.com>
> Sent: Monday, July 25, 2022 10:12 AM
> To: devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <
> isaac.w.oram@intel.com>; Desimone, Nathaniel L <
> nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>;
> Gao, Liming <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>
> Subject: [PATCH v1 2/3] UserAuthFeaturePkg: Included DXE versions are
> mutually exclusive
>
> Versions of the DXE module perform nearly identical functions, and are
> therefore mutually exclusive. Including both modules results in at least a
> duplicate HII package and an "already started" assert.
>
> Therefore, add a new PCD to determine which module will be included.
> However, now a second bug can be seen: version 2 does not challenge for
> the password. An RSC handler to hook UiApp entry is probably needed, as in
> version 1.
>
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Ankit Sinha <ankit.sinha@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
> ---
>  Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
>   | 1 +
>  Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeaturesPcd.dsc
>    | 1 +
>  Features/Intel/UserInterface/UserAuthFeaturePkg/Include/PostMemory.fdf
>   | 9 ++++++---
>  Features/Intel/UserInterface/UserAuthFeaturePkg/Include/UserAuthFeature.dsc
> | 3 +++
>  Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthFeaturePkg.dec
>   | 3 ++-
>  5 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
> b/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
> index 908fe4d1f7a9..8924dadcaa18 100644
> --- a/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
> +++ b/Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
> @@ -79,6 +79,7 @@
>    #   gLogoFeaturePkgTokenSpaceGuid.PcdLogoFeatureEnable
>       |TRUE
>  gUserAuthFeaturePkgTokenSpaceGuid.PcdUserAuthenticationFeatureEnable
> |TRUE+  gUserAuthFeaturePkgTokenSpaceGuid.PcdUseVersion1
>       |FALSE
>  gVirtualKeyboardFeaturePkgTokenSpaceGuid.PcdVirtualKeyboardFeatureEnable
> |TRUE  #diff --git
> a/Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeaturesPcd.dsc
> b/Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeaturesPcd.dsc
> index d59e3e61b9b9..5365f0a1b844 100644
> --- a/Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeaturesPcd.dsc
> +++ b/Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeaturesPcd.dsc
> @@ -75,6 +75,7 @@
>     gLogoFeaturePkgTokenSpaceGuid.PcdLogoFeatureEnable
>     |FALSE
>  gUserAuthFeaturePkgTokenSpaceGuid.PcdUserAuthenticationFeatureEnable
> |FALSE+  gUserAuthFeaturePkgTokenSpaceGuid.PcdUseVersion1
>         |FALSE
>  gVirtualKeyboardFeaturePkgTokenSpaceGuid.PcdVirtualKeyboardFeatureEnable
> |FALSE  #diff --git
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/PostMemory.fdf
> b/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/PostMemory.fdf
> index 030dcbe763d8..827deb08de7e 100644
> ---
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/PostMemory.fdf
> +++ b/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/PostMemory
> +++ .fdf
> @@ -7,6 +7,9 @@
>  # ## -INF
> UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationDxe.inf-INF
> UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthentication2Dxe.inf-INF
> UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.inf+!if
> gUserAuthFeaturePkgTokenSpaceGuid.PcdUseVersion1 == TRUE+  INF
> UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationDxe.inf+!else+
> INF
> UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthentication2Dxe.inf+!endif+
> INF
> UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.infdiff
> --git
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/UserAuthFeature.dsc
> b/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/UserAuthFeature.dsc
> index 2f39a5580caf..022678d5c852 100644
> ---
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/UserAuthFeature.dsc
> +++ b/Features/Intel/UserInterface/UserAuthFeaturePkg/Include/UserAuthFe
> +++ ature.dsc
> @@ -72,6 +72,9 @@
>    UserAuthFeaturePkg/Library/UserPasswordUiLib/UserPasswordUiLib.inf    #
> Add components here that should be included in the package build.+!if
> gUserAuthFeaturePkgTokenSpaceGuid.PcdUseVersion1 == TRUE
>  UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationDxe.inf+!else
>  UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthentication2Dxe.inf+!endif
>  UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.infdiff
> --git
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthFeaturePkg.dec
> b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthFeaturePkg.dec
> index a9174edd5486..209a0ba6a6c7 100644
> ---
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthFeaturePkg.dec
> +++ b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthFeaturePkg
> +++ .dec
> @@ -38,8 +38,9 @@
>    gUserAuthenticationGuid = { 0xee24a7f7, 0x606b, 0x4724, { 0xb3, 0xc9,
> 0xf5, 0xae, 0x4a, 0x3b, 0x81, 0x65}}  [PcdsFeatureFlag]-  ## This PCD
> specifies whether StatusCode is reported via USB3 Serial port.+  ## This
> PCD specifies whether user authentication feature is enabled.
>  gUserAuthFeaturePkgTokenSpaceGuid.PcdUserAuthenticationFeatureEnable|FALSE|BOOLEAN|0xA0000001+
> gUserAuthFeaturePkgTokenSpaceGuid.PcdUseVersion1|FALSE|BOOLEAN|0xA0000002
> [PcdsFixedAtBuild,PcdsPatchableInModule,PcdsDynamic,PcdsDynamicEx]   ##
> Indicate whether the password is cleared.--
> 2.36.1
>
>

[-- Attachment #2: Type: text/html, Size: 9367 bytes --]