From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=michael.d.kinney@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F0ABB210DF5EF for ; Tue, 7 Aug 2018 18:02:11 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Aug 2018 18:02:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,456,1526367600"; d="scan'208";a="79813400" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by orsmga001.jf.intel.com with ESMTP; 07 Aug 2018 18:02:08 -0700 Received: from orsmsx115.amr.corp.intel.com (10.22.240.11) by ORSMSX106.amr.corp.intel.com (10.22.225.133) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 7 Aug 2018 18:02:08 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.72]) by ORSMSX115.amr.corp.intel.com ([169.254.4.230]) with mapi id 14.03.0319.002; Tue, 7 Aug 2018 18:02:08 -0700 From: "Kinney, Michael D" To: "Zeng, Star" , "edk2-devel@lists.01.org" , "Kinney, Michael D" Thread-Topic: [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback Thread-Index: AQHULjlsiyBNSJAG20K6vjiLaSw0EaS1ClvQ Date: Wed, 8 Aug 2018 01:02:07 +0000 Message-ID: References: <1533637692-72548-1-git-send-email-star.zeng@intel.com> In-Reply-To: <1533637692-72548-1-git-send-email-star.zeng@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.22.254.138] MIME-Version: 1.0 Subject: Re: [PATCH] FmpDevicePkg FmpDxe: Lock variables in entrypoint instead of callback X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Aug 2018 01:02:12 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Michael D Kinney Mike > -----Original Message----- > From: Zeng, Star > Sent: Tuesday, August 7, 2018 3:28 AM > To: edk2-devel@lists.01.org > Cc: Zeng, Star ; Kinney, Michael D > > Subject: [PATCH] FmpDevicePkg FmpDxe: Lock variables in > entrypoint instead of callback >=20 > 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 =3D > gEfiEndOfDxeEventGroupGuid, the > callback's execution sequence depends on the callback's > TPL and > registration sequence. > When PcdFmpDeviceLockEventGuid =3D > gEfiEventReadyToBootGuid, the > PcdFmpDeviceLockEventGuid callback will be executed after > the > EndOfDxe callback in Variable driver, the locking will > fail. >=20 > The patch moves the variables locking logic to > entrypoint. > The patch also moves the > IsLockFmpDeviceAtLockEventGuidRequired () > checking to entrypoint. >=20 > 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. >=20 > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > --- > FmpDevicePkg/FmpDxe/FmpDxe.c | 96 > ++++++++++++++++++++++---------------------- > 1 file changed, 48 insertions(+), 48 deletions(-) >=20 > 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; >=20 > if (!mFmpDeviceLocked) { > - if (IsLockFmpDeviceAtLockEventGuidRequired ()) { > - // > - // Lock all UEFI Variables used by this module. > - // > - Status =3D LockAllFmpVariables (); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to lock > variables. Status =3D %r.\n")); > + // > + // Lock the firmware device > + // > + Status =3D FmpDeviceLock(); > + if (EFI_ERROR (Status)) { > + if (Status !=3D EFI_UNSUPPORTED) { > + DEBUG ((DEBUG_ERROR, "FmpDxe: FmpDeviceLock() > returned error. Status =3D %r\n", Status)); > } else { > - DEBUG ((DEBUG_INFO, "FmpDxe: All variables > locked\n")); > - } > - > - // > - // Lock the firmware device > - // > - Status =3D FmpDeviceLock(); > - if (EFI_ERROR (Status)) { > - if (Status !=3D EFI_UNSUPPORTED) { > - DEBUG ((DEBUG_ERROR, "FmpDxe: FmpDeviceLock() > returned error. Status =3D %r\n", Status)); > - } else { > - DEBUG ((DEBUG_WARN, "FmpDxe: FmpDeviceLock() > returned error. Status =3D %r\n", Status)); > - } > + DEBUG ((DEBUG_WARN, "FmpDxe: FmpDeviceLock() > returned error. Status =3D %r\n", Status)); > } > - mFmpDeviceLocked =3D TRUE; > - } else { > - DEBUG ((DEBUG_VERBOSE, "FmpDxe: Not calling > FmpDeviceLock() because mfg mode\n")); > } > + mFmpDeviceLocked =3D TRUE; > } > } >=20 > @@ -1417,6 +1403,45 @@ FmpDxeEntryPoint ( > // > DetectTestKey (); >=20 > + if (IsLockFmpDeviceAtLockEventGuidRequired ()) { > + // > + // Lock all UEFI Variables used by this module. > + // > + Status =3D LockAllFmpVariables (); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to lock > variables. Status =3D %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 =3D &gEfiEndOfDxeEventGroupGuid; > + if (PcdGetSize (PcdFmpDeviceLockEventGuid) =3D=3D sizeof > (EFI_GUID)) { > + LockGuid =3D (EFI_GUID *)PcdGetPtr > (PcdFmpDeviceLockEventGuid); > + } > + DEBUG ((DEBUG_INFO, "FmpDxe: Lock GUID: %g\n", > LockGuid)); > + > + Status =3D gBS->CreateEventEx ( > + EVT_NOTIFY_SIGNAL, > + TPL_CALLBACK, > + FmpDxeLockEventNotify, > + NULL, > + LockGuid, > + &mFmpDeviceLockEvent > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "FmpDxe: Failed to register > notification. Status =3D %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 ( > )); > } >=20 > - // > - // 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 =3D &gEfiEndOfDxeEventGroupGuid; > - if (PcdGetSize (PcdFmpDeviceLockEventGuid) =3D=3D sizeof > (EFI_GUID)) { > - LockGuid =3D (EFI_GUID *)PcdGetPtr > (PcdFmpDeviceLockEventGuid); > - } > - DEBUG ((DEBUG_INFO, "FmpDxe: Lock GUID: %g\n", > LockGuid)); > - > - Status =3D 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 =3D %r\n", Status)); > - } > - ASSERT_EFI_ERROR (Status); > - > return Status; > } > -- > 2.7.0.windows.1