Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency

["Zeng, Star" <star.zeng@intel.com>]

Good to see the series pushed.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, October 10, 2017 6:09 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Dong, Eric <eric.dong@intel.com>; Ladi Prosek <lprosek@redhat.com>
Subject: Re: [edk2] [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency

On 10/10/17 06:17, Yao, Jiewen wrote:
> Thanks.
> Please use ASSERT() when you check in.
>
> Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>

Thank you all for the (quick!) feedback; I've pushed the series:
35ac962b5473..fda8f631edbb.

Here's the cumulative diff between the posted (v1) and the pushed series, based on the comments:

> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index 759e47db7f29..b98b8556a23a 100644
> --- 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.
> +++ h
> @@ -62,9 +62,7 @@ MorLockInitAtEndOfDxe (
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index a91fc42ff465..7142e2da2073 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -31,9 +31,7 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL     mVariableLock;
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 0a0281e44bc1..93a300a84677 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -319,9 +319,7 @@ SetVariableCheckHandlerMorLock (
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> @@ -427,8 +425,9 @@ MorLockInitAtEndOfDxe (
>    if (!mMorLockInitializationRequired) {
>      //
>      // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> -    // the variable write service is unavailable. Do nothing.
> +    // the variable write service is unavailable. This should never happen.
>      //
> +    ASSERT (FALSE);
>      return;
>    }
>

In addition, I modified the commit message of patch #2 (now commit 03877377e326, "MdeModulePkg/Variable/RuntimeDxe: move MOR func.
declarations to header", 2017-09-30), to credit Star for the fixed "Attributes" param description.

(The ASSERT from Jiewen needed no commit message update; commit 7516532f9c2d ("MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe", 2017-09-30) already carried "Suggested-by: Jiewen Yao
<jiewen.yao@intel.com>".)


I plan to post the separate patch for option (a) soon (see <http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A9D71F4@shsmsx102.ccr.corp.intel.com>).

Thank you!
Laszlo