Re: [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, August 7, 2018 3:28 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH] FmpDevicePkg FmpDxe: Lock variables in
> entrypoint instead of callback
> 
> Current code locks variables in PcdFmpDeviceLockEventGuid
> callback by
> VariableLock protocol whose interface will be closed at
> EndOfDxe.
> So the PcdFmpDeviceLockEventGuid callback needs be
> executed before
> the EndOfDxe callback in Variable driver.
> When PcdFmpDeviceLockEventGuid =
> gEfiEndOfDxeEventGroupGuid, the
> callback's execution sequence depends on the callback's
> TPL and
> registration sequence.
> When PcdFmpDeviceLockEventGuid =
> gEfiEventReadyToBootGuid, the
> PcdFmpDeviceLockEventGuid callback will be executed after
> the
> EndOfDxe callback in Variable driver, the locking will
> fail.
> 
> The patch moves the variables locking logic to
> entrypoint.
> The patch also moves the
> IsLockFmpDeviceAtLockEventGuidRequired ()
> checking to entrypoint.
> 
> The entrypoint's final return status should be better to
> depend on
> the return status of
> RegisterFmpInstaller/InstallFmpInstance, but not
> gBS->CreateEventEx.
> So the patch also moves the
> RegisterFmpInstaller/InstallFmpInstance
> calling to the end of entrypoint.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  FmpDevicePkg/FmpDxe/FmpDxe.c | 96
> ++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 48 deletions(-)
> 
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c
> b/FmpDevicePkg/FmpDxe/FmpDxe.c
> index 3794ac5008f9..57eac5ac147f 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> @@ -1248,32 +1248,18 @@ FmpDxeLockEventNotify (
>    EFI_STATUS  Status;
> 
>    if (!mFmpDeviceLocked) {
> -    if (IsLockFmpDeviceAtLockEventGuidRequired ()) {
> -      //
> -      // Lock all UEFI Variables used by this module.
> -      //
> -      Status = LockAllFmpVariables ();
> -      if (EFI_ERROR (Status)) {
> -        DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to lock
> variables.  Status = %r.\n"));
> +    //
> +    // Lock the firmware device
> +    //
> +    Status = FmpDeviceLock();
> +    if (EFI_ERROR (Status)) {
> +      if (Status != EFI_UNSUPPORTED) {
> +        DEBUG ((DEBUG_ERROR, "FmpDxe: FmpDeviceLock()
> returned error.  Status = %r\n", Status));
>        } else {
> -        DEBUG ((DEBUG_INFO, "FmpDxe: All variables
> locked\n"));
> -      }
> -
> -      //
> -      // Lock the firmware device
> -      //
> -      Status = FmpDeviceLock();
> -      if (EFI_ERROR (Status)) {
> -        if (Status != EFI_UNSUPPORTED) {
> -          DEBUG ((DEBUG_ERROR, "FmpDxe: FmpDeviceLock()
> returned error.  Status = %r\n", Status));
> -        } else {
> -          DEBUG ((DEBUG_WARN, "FmpDxe: FmpDeviceLock()
> returned error.  Status = %r\n", Status));
> -        }
> +        DEBUG ((DEBUG_WARN, "FmpDxe: FmpDeviceLock()
> returned error.  Status = %r\n", Status));
>        }
> -      mFmpDeviceLocked = TRUE;
> -    } else {
> -      DEBUG ((DEBUG_VERBOSE, "FmpDxe: Not calling
> FmpDeviceLock() because mfg mode\n"));
>      }
> +    mFmpDeviceLocked = TRUE;
>    }
>  }
> 
> @@ -1417,6 +1403,45 @@ FmpDxeEntryPoint (
>    //
>    DetectTestKey ();
> 
> +  if (IsLockFmpDeviceAtLockEventGuidRequired ()) {
> +    //
> +    // Lock all UEFI Variables used by this module.
> +    //
> +    Status = LockAllFmpVariables ();
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to lock
> variables.  Status = %r.\n", Status));
> +    } else {
> +      DEBUG ((DEBUG_INFO, "FmpDxe: All variables
> locked\n"));
> +    }
> +
> +    //
> +    // Register notify function to lock the FMP device.
> +    // The lock event GUID is retrieved from
> PcdFmpDeviceLockEventGuid.
> +    // If PcdFmpDeviceLockEventGuid is not the size of
> an EFI_GUID, then
> +    // gEfiEndOfDxeEventGroupGuid is used.
> +    //
> +    LockGuid = &gEfiEndOfDxeEventGroupGuid;
> +    if (PcdGetSize (PcdFmpDeviceLockEventGuid) == sizeof
> (EFI_GUID)) {
> +      LockGuid = (EFI_GUID *)PcdGetPtr
> (PcdFmpDeviceLockEventGuid);
> +    }
> +    DEBUG ((DEBUG_INFO, "FmpDxe: Lock GUID: %g\n",
> LockGuid));
> +
> +    Status = gBS->CreateEventEx (
> +                    EVT_NOTIFY_SIGNAL,
> +                    TPL_CALLBACK,
> +                    FmpDxeLockEventNotify,
> +                    NULL,
> +                    LockGuid,
> +                    &mFmpDeviceLockEvent
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to register
> notification.  Status = %r\n", Status));
> +    }
> +    ASSERT_EFI_ERROR (Status);
> +  } else {
> +    DEBUG ((DEBUG_VERBOSE, "FmpDxe: Not registering
> notification to call FmpDeviceLock() because mfg
> mode\n"));
> +  }
> +
>    //
>    // Register with library the install function so if
> the library uses
>    // UEFI driver model/driver binding protocol it can
> install FMP on its device handle
> @@ -1436,30 +1461,5 @@ FmpDxeEntryPoint (
>        ));
>    }
> 
> -  //
> -  // Register notify function to lock the FMP device.
> -  // The lock event GUID is retrieved from
> PcdFmpDeviceLockEventGuid.
> -  // If PcdFmpDeviceLockEventGuid is not the size of an
> EFI_GUID, then
> -  // gEfiEndOfDxeEventGroupGuid is used.
> -  //
> -  LockGuid = &gEfiEndOfDxeEventGroupGuid;
> -  if (PcdGetSize (PcdFmpDeviceLockEventGuid) == sizeof
> (EFI_GUID)) {
> -    LockGuid = (EFI_GUID *)PcdGetPtr
> (PcdFmpDeviceLockEventGuid);
> -  }
> -  DEBUG ((DEBUG_INFO, "FmpDxe: Lock GUID: %g\n",
> LockGuid));
> -
> -  Status = gBS->CreateEventEx (
> -                  EVT_NOTIFY_SIGNAL,
> -                  TPL_CALLBACK,
> -                  FmpDxeLockEventNotify,
> -                  NULL,
> -                  LockGuid,
> -                  &mFmpDeviceLockEvent
> -                  );
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to register for
> ready to boot.  Status = %r\n", Status));
> -  }
> -  ASSERT_EFI_ERROR (Status);
> -
>    return Status;
>  }
> --
> 2.7.0.windows.1