From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.11526.1650480142262674401 for ; Wed, 20 Apr 2022 11:42:22 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=gOs8MJM/; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.46.36]) by linux.microsoft.com (Postfix) with ESMTPSA id 7F26E2056561; Wed, 20 Apr 2022 11:42:20 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 7F26E2056561 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1650480141; bh=gKuw8m3DWlvitTwtEUGMiqAhOSYhhnSJniiwSoNk7VA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=gOs8MJM/n4p74Wc0GpuC86M5wpUYgKTdB3QfnHmvRommw2PzosmMAmxj7dYvh2RVg 64d50Ci11rkRF7SyLVPXa2dmcfoEDw/mwD1yZfmMYxvbiZRQ4DARdvznHGaMwZ1mCy GIdIswf/i+09yUNk8F1y+4AIHtWy0bl612UKcguQ= Message-ID: Date: Wed, 20 Apr 2022 14:42:19 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Use SmmWaitForAllProcessor() in VariableSmm driver. To: devel@edk2.groups.io, zhihao.li@intel.com Cc: Jian J Wang , Liming Gao , Siyuan Fu , Ni Ray , Sami Mujawar , Ilias Apalodimas , Ard Biesheuvel , Leif Lindholm References: <20220420173201.1050-1-zhihao.li@intel.com> From: "Michael Kubacki" In-Reply-To: <20220420173201.1050-1-zhihao.li@intel.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable If I understand this patch correctly, it is exactly duplicating the=20 SmmCpuRendezvousLib library class/interface in ModeModulePkg because=20 code there cannot depend on the library class/interface definition=20 currently in UefiCpuPkg: https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Include/Library/= SmmCpuRendezvousLib.h If that's the case, this is creating maintenance debt by requiring the=20 two interfaces always be kept in sync and developer confusion. It is okay to have an interface defined in a more broadly scoped package=20 (e.g. MdePkg) with instances implemented in other packages. For example, the HobLib interface is defined in MdePkg: https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/HobL= ib.h But, instances are described in many other packages including a NULL=20 instance in MdeModulePkg: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/BaseHo= bLibNull/BaseHobLibNull.inf And a Standalone MM instance in StandaloneMmPkg: https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/Sta= ndaloneMmHobLib/StandaloneMmHobLib.inf If this interface is actually consumed by MdeModulePkg, the interface=20 should be defined in a single package that is allowed to be a dependency=20 for MdeModulePkg. The NULL library instance referenced in the=20 MdeModulePkg build should also be implemented in an allowed package. The library interface should be removed from other packages=20 (UefiCpuPkg). Other library instances can then be implemented elsewhere=20 using the library class interface from the singly defined location. Regards, Michael On 4/20/2022 1:32 PM, Li, Zhihao wrote: > REF=EF=BC=9A https://bugzilla.tianocore.org/show_bug.cgi?id=3D3854 >=20 > In UefiCpuPkg, there are a new Protocol with the new service > SmmWaitForAllProcessor(), which can be used by SMI handler > to optionally wait for other APs to complete SMM rendezvous in > relaxed AP mode. >=20 > This patch use the new service to let VariableSmm driver work > normally in relaxed AP mode. >=20 > Due to MdeModulePkg can not depend on UefiCpuPkg, use null version > implementation in MdeModulePkg.dsc. >=20 > Cc: Jian J Wang > Cc: Liming Gao > Cc: Siyuan Fu > Cc: Ni Ray > Cc: Sami Mujawar > Cc: Ilias Apalodimas > Cc: Ard Biesheuvel > Cc: Leif Lindholm >=20 > Signed-off-by: Zhihao Li > --- > MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.= c | 29 ++++++++++++++++++++ > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c = | 10 ++++++- > MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h = | 27 ++++++++++++++++++ > MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.= inf | 27 ++++++++++++++++++ > MdeModulePkg/MdeModulePkg.dec = | 5 +++- > MdeModulePkg/MdeModulePkg.dsc = | 5 +++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf = | 3 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf = | 3 +- > 8 files changed, 104 insertions(+), 5 deletions(-) >=20 > diff --git a/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezv= ousLibNull.c b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezv= ousLibNull.c > new file mode 100644 > index 000000000000..474195bbb374 > --- /dev/null > +++ b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibN= ull.c > @@ -0,0 +1,29 @@ > +/** @file >=20 > + SMM CPU Rendezvous sevice implement. >=20 > + >=20 > + Copyright (c) 2022, Intel Corporation. All rights reserved.
>=20 > + SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > + >=20 > +**/ >=20 > + >=20 > +#include >=20 > +#include >=20 > + >=20 > +/** >=20 > + This routine wait for all AP processors to arrive in SMM. >=20 > + >=20 > + @param[in] BlockingMode Blocking mode or non-blocking mode. >=20 > + >=20 > + @retval EFI_SUCCESS All avaiable APs arrived. >=20 > + @retval EFI_TIMEOUT Wait for all APs until timeout. >=20 > + @retval OTHER Fail to register SMM CPU Rendezvous service Pro= tocol. >=20 > +**/ >=20 > +EFI_STATUS >=20 > +EFIAPI >=20 > +SmmWaitForAllProcessor ( >=20 > + IN BOOLEAN BlockingMode >=20 > + ) >=20 > +{ >=20 > + ASSERT (FALSE); >=20 > + return EFI_SUCCESS; >=20 > +} >=20 > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b= /MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > index 517cae7b00f8..52a9b0e6b202 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > @@ -14,7 +14,7 @@ > VariableServiceSetVariable(), VariableServiceQueryVariableInfo(), R= eclaimForOS(), >=20 > SmmVariableGetStatistics() should also do validation based on its o= wn knowledge. >=20 > =20 >=20 > -Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.
>=20 > +Copyright (c) 2010 - 2022, Intel Corporation. All rights reserved.
>=20 > Copyright (c) 2018, Linaro, Ltd. All rights reserved.
>=20 > SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > =20 >=20 > @@ -28,6 +28,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > =20 >=20 > #include >=20 > #include >=20 > +#include >=20 > =20 >=20 > #include >=20 > #include "Variable.h" >=20 > @@ -656,6 +657,13 @@ SmmVariableHandler ( > goto EXIT; >=20 > } >=20 > =20 >=20 > + if ((SmmVariableHeader->Attributes & EFI_VARIABLE_NON_VOLATILE) = !=3D 0) { >=20 > + if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) { >=20 > + DEBUG ((DEBUG_ERROR, "SetVariable: fail to wait for all AP c= heck in SMM!\n")); >=20 > + goto EXIT; >=20 > + } >=20 > + } >=20 > + >=20 > Status =3D VariableServiceSetVariable ( >=20 > SmmVariableHeader->Name, >=20 > &SmmVariableHeader->Guid, >=20 > diff --git a/MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h b/MdeMo= dulePkg/Include/Library/SmmCpuRendezvousLib.h > new file mode 100644 > index 000000000000..82e459e9106e > --- /dev/null > +++ b/MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h > @@ -0,0 +1,27 @@ > +/** @file >=20 > + SMM CPU Rendezvous library header file. >=20 > + >=20 > + Copyright (c) 2022, Intel Corporation. All rights reserved.
>=20 > + SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > + >=20 > +**/ >=20 > + >=20 > +#ifndef SMM_CPU_RENDEZVOUS_H_ >=20 > +#define SMM_CPU_RENDEZVOUS_H_ >=20 > + >=20 > +/** >=20 > + This routine wait for all AP processors to arrive in SMM. >=20 > + >=20 > + @param[in] BlockingMode Blocking mode or non-blocking mode. >=20 > + >=20 > + @retval EFI_SUCCESS All processors checked in to SMM. >=20 > + @retval EFI_TIMEOUT Wait for all APs until timeout. >=20 > + >=20 > +**/ >=20 > +EFI_STATUS >=20 > +EFIAPI >=20 > +SmmWaitForAllProcessor ( >=20 > + IN BOOLEAN BlockingMode >=20 > + ); >=20 > + >=20 > +#endif >=20 > diff --git a/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezv= ousLibNull.inf b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRende= zvousLibNull.inf > new file mode 100644 > index 000000000000..0bd4f39e7277 > --- /dev/null > +++ b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibN= ull.inf > @@ -0,0 +1,27 @@ > +## @file >=20 > +# SMM CPU Rendezvous service lib. >=20 > +# >=20 > +# This is SMM CPU rendezvous service lib that wait for all >=20 > +# APs to enter SMM mode. >=20 > +# >=20 > +# Copyright (c) 2022, Intel Corporation. All rights reserved.
>=20 > +# SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > +# >=20 > +## >=20 > + >=20 > +[Defines] >=20 > + INF_VERSION =3D 0x00010005 >=20 > + BASE_NAME =3D SmmCpuRendezvousLibNull >=20 > + FILE_GUID =3D 1e5790ea-d013-4d7b-9047-b4342a762= 027 >=20 > + MODULE_TYPE =3D DXE_SMM_DRIVER >=20 > + LIBRARY_CLASS =3D SmmCpuRendezvousLib|MM_STANDALONE= DXE_SMM_DRIVER >=20 > + >=20 > +[Sources] >=20 > + SmmCpuRendezvousLibNull.c >=20 > + >=20 > +[Packages] >=20 > + MdePkg/MdePkg.dec >=20 > + MdeModulePkg/MdeModulePkg.dec >=20 > + >=20 > +[LibraryClasses] >=20 > + DebugLib >=20 > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.= dec > index 463e889e9a68..06ada41b7344 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -4,7 +4,7 @@ > # and libraries instances, which are used for those modules. >=20 > # >=20 > # Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. >=20 > -# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved. >=20 > +# Copyright (c) 2007 - 2022, Intel Corporation. All rights reserved. >=20 > # Copyright (c) 2016, Linaro Ltd. All rights reserved.
>=20 > # (C) Copyright 2016 - 2019 Hewlett Packard Enterprise Development LP=
>=20 > # Copyright (c) 2017, AMD Incorporated. All rights reserved.
>=20 > @@ -154,6 +154,9 @@ > # >=20 > VariablePolicyHelperLib|Include/Library/VariablePolicyHelperLib.h >=20 > =20 >=20 > + ## @libraryclass Provides function for SMM CPU Rendezvous Library. >=20 > + SmmCpuRendezvousLib|Include/Library/SmmCpuRendezvousLib.h >=20 > + >=20 > [Guids] >=20 > ## MdeModule package token space guid >=20 > # Include/Guid/MdeModulePkgTokenSpace.h >=20 > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.= dsc > index b1d83461865e..4580320a7230 100644 > --- a/MdeModulePkg/MdeModulePkg.dsc > +++ b/MdeModulePkg/MdeModulePkg.dsc > @@ -2,7 +2,7 @@ > # EFI/PI Reference Module Package for All Architectures >=20 > # >=20 > # (C) Copyright 2014 Hewlett-Packard Development Company, L.P.
>=20 > -# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved. >=20 > +# Copyright (c) 2007 - 2022, Intel Corporation. All rights reserved. >=20 > # Copyright (c) Microsoft Corporation. >=20 > # >=20 > # SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > @@ -152,6 +152,7 @@ > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesT= ableLib.inf >=20 > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.inf >=20 > SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf >=20 > + SmmCpuRendezvousLib|MdeModulePkg/Library/SmmCpuRendezvousLibNull/Smm= CpuRendezvousLibNull.inf >=20 > =20 >=20 > [LibraryClasses.common.UEFI_DRIVER] >=20 > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf >=20 > @@ -172,6 +173,7 @@ > MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/Stan= daloneMmServicesTableLib.inf >=20 > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxStandaloneM= mLib.inf >=20 > MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLi= b.inf >=20 > + SmmCpuRendezvousLib|MdeModulePkg/Library/SmmCpuRendezvousLibNull/Smm= CpuRendezvousLibNull.inf >=20 > =20 >=20 > [LibraryClasses.ARM, LibraryClasses.AARCH64] >=20 > ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf >=20 > @@ -459,6 +461,7 @@ > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStan= daloneMm.inf >=20 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf >=20 > !endif >=20 > + MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull= .inf >=20 > =20 >=20 > [Components.IA32, Components.X64] >=20 > MdeModulePkg/Universal/DebugSupportDxe/DebugSupportDxe.inf >=20 > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf= b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > index eaa97a01c6e5..0bebd92b1626 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 pr= otect these resources, >=20 > # the authentication service provided in this driver will be broken,= and the behavior is undefined. >=20 > # >=20 > -# Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved. >=20 > +# Copyright (c) 2010 - 2022, Intel Corporation. All rights reserved. >=20 > # Copyright (c) Microsoft Corporation. >=20 > # SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > # >=20 > @@ -82,6 +82,7 @@ > UefiBootServicesTableLib >=20 > VariablePolicyLib >=20 > VariablePolicyHelperLib >=20 > + SmmCpuRendezvousLib >=20 > =20 >=20 > [Protocols] >=20 > gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES >=20 > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandal= oneMm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandalone= Mm.inf > index d8c4f77e7f1f..595baaf70164 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i= nf > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i= nf > @@ -18,7 +18,7 @@ > # may not be modified without authorization. If platform fails to pr= otect these resources, >=20 > # the authentication service provided in this driver will be broken,= and the behavior is undefined. >=20 > # >=20 > -# Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved. >=20 > +# Copyright (c) 2010 - 2022, Intel Corporation. All rights reserved. >=20 > # Copyright (c) 2018, Linaro, Ltd. All rights reserved.
>=20 > # Copyright (c) Microsoft Corporation. >=20 > # SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > @@ -78,6 +78,7 @@ > VarCheckLib >=20 > VariablePolicyLib >=20 > VariablePolicyHelperLib >=20 > + SmmCpuRendezvousLib >=20 > =20 >=20 > [Protocols] >=20 > gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES >=20