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

Fri May 19 09:00:18 UTC 2023

Ray and Gerd:
  Can you give the comments for this change?

Thanks
Liming
> -----邮件原件-----
> 发件人: Li, Zhihao <zhihao.li at intel.com>
> 发送时间: 2023年5月19日 16:11
> 收件人: Gao, Liming <gaoliming at byosoft.com.cn>; devel at edk2.groups.io; Ni,
> Ray <ray.ni at intel.com>; kraxel at redhat.com
> 抄送: Wang, Jian J <jian.j.wang at intel.com>
> 主题: RE: [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 at byosoft.com.cn>
> Sent: Thursday, May 18, 2023 5:38 PM
> To: Li, Zhihao <zhihao.li at intel.com>; devel at edk2.groups.io; Ni, Ray
> <ray.ni at intel.com>; kraxel at redhat.com
> Cc: Wang, Jian J <jian.j.wang at 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 at intel.com>
> > 发送时间: 2023年5月10日 18:57
> > 收件人: devel at edk2.groups.io
> > 抄送: Jian J Wang <jian.j.wang at intel.com>; Liming Gao
> > <gaoliming at 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 at intel.com>
> > Cc: Liming Gao <gaoliming at byosoft.com.cn>
> >
> > Signed-off-by: Zhihao Li <zhihao.li at 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.in
> > f
> >
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in
> > f
> > index f09bed40cf51..89187456ca25 100644
> > ---
> >
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in
> > 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
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105072): https://edk2.groups.io/g/devel/message/105072
Mute This Topic: https://groups.io/mt/99008674/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-