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

[Laszlo Ersek <lersek@redhat.com>]

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