Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous check before SmmSetVariable.

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Hi All, 

I think we need this patch:

There is a requirement that all CPU threads must in SMM for Non-Volatile variable. Because the SMM will disables the flash protection. Before that, we must guarantee all CPU threads are in SMM to avoid the non-smm mode cpus modify the flash.


Zhihao,

I think this is only needed for the Non-Volatile, I suggest as below check:

      if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
        if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
          DEBUG ((DEBUG_ERROR, " SmmVariableSetVariable: Fail to wait for all AP check in SMM!\n"));
          Status = EFI_ABORTED;
          goto EXIT;
        }
      }

Thanks,
Jiaxin

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Li,
> Zhihao
> Sent: Friday, May 19, 2023 4:11 PM
> To: Gao, Liming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Ni,
> Ray <ray.ni@intel.com>; kraxel@redhat.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> add Ap rendezvous check before SmmSetVariable.
> 
> Hi Liming
> In Ap-Relaxed mode, Bsp will not wait for all ap arrive and call the SMI
> handlers.  But some SMI handlers need all Aps arrive in smm mode such as
> SmmSetVariable. As the design, SetVariable need to let all aps arrive because
> it will write flash. Half year ago, I send the patch that calling
> SmmCpuRendezvous() before SmmSetVariable. It was reviewed but hasn't
> merged. SmmCpuRendezvous() will return immediately in traditional-AP
> mode.
> I'm not sure what returns EFI_ACCESS_DENIED. Calling SmmCpuRendezvous()
> before SmmSetVariable is our original design but haven't implemented.
> 
> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Thursday, May 18, 2023 5:38 PM
> To: Li, Zhihao <zhihao.li@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>; kraxel@redhat.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: 回复: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap
> rendezvous check before SmmSetVariable.
> 
> Zhihao:
>   Have you root cause this issue that SmmVariableSetVariable may return
> EFI_ACCESS_DENIED?
> 
>   I am not sure whether this fix is proper. I also add UefiCpuPkg maintainers
> Ray and Gerd in the mail loop for this discussion.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: Zhihao Li <zhihao.li@intel.com>
> > 发送时间: 2023年5月10日 18:57
> > 收件人: devel@edk2.groups.io
> > 抄送: Jian J Wang <jian.j.wang@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>
> > 主题: [PATCH v1 1/1] MdeModulePkg/VariableSmm.c: add Ap rendezvous
> check
> > before SmmSetVariable.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4429
> >
> > For Ap-Relaxed sync mode, SmmVariableSetVariable() need to let all Aps
> > arrive to smm before it set the variable. If not, it would return
> > EFI_ACCESS_DENIED.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >
> > Signed-off-by: Zhihao Li <zhihao.li@intel.com>
> > ---
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > | 10 +++++++++-
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > |  3 ++-
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> > |  3 ++-
> >  3 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > index 5253c328dcd9..4944903e64d4 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > @@ -14,7 +14,7 @@
> >    VariableServiceSetVariable(), VariableServiceQueryVariableInfo(),
> > ReclaimForOS(),
> >
> >    SmmVariableGetStatistics() should also do validation based on its
> > own knowledge.
> >
> >
> >
> > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> >
> > +Copyright (c) 2010 - 2023, Intel Corporation. All rights
> > +reserved.<BR>
> >
> >  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > @@ -28,6 +28,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >  #include <Library/MmServicesTableLib.h>
> >
> >  #include <Library/VariablePolicyLib.h>
> >
> > +#include <Library/SmmCpuRendezvousLib.h>
> >
> >
> >
> >  #include <Guid/SmmVariableCommon.h>
> >
> >  #include "Variable.h"
> >
> > @@ -87,6 +88,13 @@ SmmVariableSetVariable (  {
> >
> >    EFI_STATUS  Status;
> >
> >
> >
> > +  //
> >
> > +  // Need to wait for all Aps to arrive in Relaxed-AP Sync Mode
> >
> > +  //
> >
> > +  if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
> >
> > +    DEBUG ((DEBUG_ERROR, "SetVariable: fail to wait for all AP check
> > + in
> > SMM!\n"));
> >
> > +  }
> >
> > +
> >
> >    //
> >
> >    // Disable write protection when the calling SetVariable() through
> > EFI_SMM_VARIABLE_PROTOCOL.
> >
> >    //
> >
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > index 8c552b87e080..1cf0d051e6c9 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > @@ -18,7 +18,7 @@
> >  #  may not be modified without authorization. If platform fails to
> protect
> > these resources,
> >
> >  #  the authentication service provided in this driver will be broken,
> > and
> the
> > behavior is undefined.
> >
> >  #
> >
> > -# Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> >
> > +# Copyright (c) 2010 - 2023, Intel Corporation. All rights
> > +reserved.<BR>
> >
> >  # Copyright (c) Microsoft Corporation.
> >
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  #
> >
> > @@ -84,6 +84,7 @@
> >    VariablePolicyLib
> >
> >    VariablePolicyHelperLib
> >
> >    SafeIntLib
> >
> > +  SmmCpuRendezvousLib
> >
> >
> >
> >  [Protocols]
> >
> >    gEfiSmmFirmwareVolumeBlockProtocolGuid        ## CONSUMES
> >
> > diff --git
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> n
> > f
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> in
> > f
> > index f09bed40cf51..89187456ca25 100644
> > ---
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> n
> > f
> > +++
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.
> in
> > f
> > @@ -18,7 +18,7 @@
> >  #  may not be modified without authorization. If platform fails to
> protect
> > these resources,
> >
> >  #  the authentication service provided in this driver will be broken,
> > and
> the
> > behavior is undefined.
> >
> >  #
> >
> > -# Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> >
> > +# Copyright (c) 2010 - 2023, Intel Corporation. All rights
> > +reserved.<BR>
> >
> >  # Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
> >
> >  # Copyright (c) Microsoft Corporation.
> >
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -80,6 +80,7 @@
> >    VariableFlashInfoLib
> >
> >    VariablePolicyLib
> >
> >    VariablePolicyHelperLib
> >
> > +  SmmCpuRendezvousLib
> >
> >
> >
> >  [Protocols]
> >
> >    gEfiSmmFirmwareVolumeBlockProtocolGuid        ## CONSUMES
> >
> > --
> > 2.26.2.windows.1
> 
> 
> 
> 
> 
> 
>