* [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback
@ 2018-08-07 10:28 Star Zeng
2018-08-08 1:02 ` Kinney, Michael D
0 siblings, 1 reply; 2+ messages in thread
From: Star Zeng @ 2018-08-07 10:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Michael D Kinney
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback
2018-08-07 10:28 [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback Star Zeng
@ 2018-08-08 1:02 ` Kinney, Michael D
0 siblings, 0 replies; 2+ messages in thread
From: Kinney, Michael D @ 2018-08-08 1:02 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org, Kinney, Michael D
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-08-08 1:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-07 10:28 [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback Star Zeng
2018-08-08 1:02 ` Kinney, Michael D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox