Thanks Jiewen & Ray.
Zhihao, could you try the suggestion from Jiewen? You can sync with me for
detail.
Thanks,
Jiaxin
> -----Original Message-----
> From: Ni, Ray <ray...@intel.com>
> Sent: Thursday, June 1, 2023 9:07 AM
> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Wu, Jiaxin
> <jiaxin...@intel.com>; Li, Zhihao <zhihao...@intel.com>; Gao, Liming
> <gaolim...@byosoft.com.cn>; kra...@redhat.com
> Cc: Wang, Jian J <jian.j.w...@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> add Ap rendezvous check before SmmSetVariable.
>
> Jiewen,
> Good suggestion😊.
> This also resolve's Jiaxin's comments that check if the variable is
> non-volatile.
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen....@intel.com>
> > Sent: Thursday, June 1, 2023 9:06 AM
> > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin...@intel.com>; Li, Zhihao
> > <zhihao...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Ni, Ray
> > <ray...@intel.com>; kra...@redhat.com
> > Cc: Wang, Jian J <jian.j.w...@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> > add Ap rendezvous check before SmmSetVariable.
> >
> > Disabling flash is a silicon behavior.
> >
> > Can we do SmmWaitForAllProcessor() in FVB driver, or SPI driver ?
> >
> > Thank you
> > Yao, Jiewen
> >
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> > Jiaxin
> > > Sent: Thursday, June 1, 2023 9:03 AM
> > > To: devel@edk2.groups.io; Li, Zhihao <zhihao...@intel.com>; Gao, Liming
> > > <gaolim...@byosoft.com.cn>; Ni, Ray <ray...@intel.com>;
> > kra...@redhat.com
> > > Cc: Wang, Jian J <jian.j.w...@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/VariableSmm.c:
> > add
> > > Ap rendezvous check before SmmSetVariable.
> > >
> > > 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 <gaolim...@byosoft.com.cn>; devel@edk2.groups.io;
> Ni,
> > > > Ray <ray...@intel.com>; kra...@redhat.com
> > > > Cc: Wang, Jian J <jian.j.w...@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 <gaolim...@byosoft.com.cn>
> > > > Sent: Thursday, May 18, 2023 5:38 PM
> > > > To: Li, Zhihao <zhihao...@intel.com>; devel@edk2.groups.io; Ni, Ray
> > > > <ray...@intel.com>; kra...@redhat.com
> > > > Cc: Wang, Jian J <jian.j.w...@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...@intel.com>
> > > > > 发送时间: 2023年5月10日 18:57
> > > > > 收件人: devel@edk2.groups.io
> > > > > 抄送: Jian J Wang <jian.j.w...@intel.com>; Liming Gao
> > > > > <gaolim...@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.w...@intel.com>
> > > > > Cc: Liming Gao <gaolim...@byosoft.com.cn>
> > > > >
> > > > > Signed-off-by: Zhihao Li <zhihao...@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105521): https://edk2.groups.io/g/devel/message/105521
Mute This Topic: https://groups.io/mt/99008324/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
