Laszlo, There are 2 reasons to list all source files used in a module build in the [Sources] section.
1) Support incremental builds. If a change to the .h file is made, then the module may not be rebuilt if the .h file is not listed in [Sources] 2) Support of UEFI Distribution Package distribution format. The UPT tools that creates UDP packages uses the [Sources] section for the inventory of files. If a file is missing, then it will not be included in the UDP file. Use of STATIC in OvmfPkg is ok. I only mentioned it to be consistent with other packages. I mention global variable location to be consistent with other packages. The preference is to put all globals before function implementations within a C file, so they are not mixed between functions and are harder to find. The ctags issue is interesting. I wonder if anyone else has found a way around that issue. Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Monday, November 23, 2015 4:14 AM > To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-de...@ml01.01.org > Subject: Re: [edk2] [PATCH v4 08/41] OvmfPkg: add DXE_DRIVER for providing > TSEG-as-SMRAM during boot-time DXE > > On 11/21/15 07:03, Kinney, Michael D wrote: > > Laszlo, > > > > Minor comments included below. > > > > Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> > > Thank you. I'll pick up your R-b, but I'll also respond to your comments > below: > > >> -----Original Message----- > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > >> Of Laszlo Ersek > >> Sent: Tuesday, November 3, 2015 1:01 PM > >> To: edk2-de...@ml01.01.org > >> Subject: [edk2] [PATCH v4 08/41] OvmfPkg: add DXE_DRIVER for > >> providing TSEG-as-SMRAM during boot-time DXE > >> > >> The SMM core depends on EFI_SMM_ACCESS2_PROTOCOL. This small driver > >> (which is a thin wrapper around "OvmfPkg/SmmAccess/SmramInternal.c" that > >> was added in the previous patch) provides that > protocol. > >> > >> Notably, EFI_SMM_ACCESS2_PROTOCOL is for boot time only, therefore our > >> MODULE_TYPE is not DXE_RUNTIME_DRIVER. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> > >> --- > >> OvmfPkg/OvmfPkgIa32.dsc | 4 + > >> OvmfPkg/OvmfPkgIa32X64.dsc | 4 + > >> OvmfPkg/OvmfPkgX64.dsc | 4 + > >> OvmfPkg/OvmfPkgIa32.fdf | 4 + > >> OvmfPkg/OvmfPkgIa32X64.fdf | 4 + > >> OvmfPkg/OvmfPkgX64.fdf | 4 + > >> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 57 +++++++ > >> OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 156 ++++++++++++++++++++ > >> 8 files changed, 237 insertions(+) > >> > >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index > >> 0b729ca..d7bc38d 100644 > >> --- a/OvmfPkg/OvmfPkgIa32.dsc > >> +++ b/OvmfPkg/OvmfPkgIa32.dsc > >> @@ -672,3 +672,7 @@ [Components] > >> !endif > >> > >> OvmfPkg/PlatformDxe/Platform.inf > >> + > >> +!if $(SMM_REQUIRE) == TRUE > >> + OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> +!endif > >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > >> index 7f672d9..e17cbe5 100644 > >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc > >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > >> @@ -679,3 +679,7 @@ [Components.X64] > >> !endif > >> > >> OvmfPkg/PlatformDxe/Platform.inf > >> + > >> +!if $(SMM_REQUIRE) == TRUE > >> + OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> +!endif > >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index > >> 986c019..a748fb3 100644 > >> --- a/OvmfPkg/OvmfPkgX64.dsc > >> +++ b/OvmfPkg/OvmfPkgX64.dsc > >> @@ -677,3 +677,7 @@ [Components] > >> !endif > >> > >> OvmfPkg/PlatformDxe/Platform.inf > >> + > >> +!if $(SMM_REQUIRE) == TRUE > >> + OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> +!endif > >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index > >> 650dab1..285720f 100644 > >> --- a/OvmfPkg/OvmfPkgIa32.fdf > >> +++ b/OvmfPkg/OvmfPkgIa32.fdf > >> @@ -355,6 +355,10 @@ [FV.DXEFV] > >> INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > >> INF OvmfPkg/PlatformDxe/Platform.inf > >> > >> +!if $(SMM_REQUIRE) == TRUE > >> +INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> +!endif > >> + > >> > >> ##################################################################### > >> ########### > >> > >> [FV.FVMAIN_COMPACT] > >> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > >> index 5830401..02e8752 100644 > >> --- a/OvmfPkg/OvmfPkgIa32X64.fdf > >> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > >> @@ -355,6 +355,10 @@ [FV.DXEFV] > >> INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > >> INF OvmfPkg/PlatformDxe/Platform.inf > >> > >> +!if $(SMM_REQUIRE) == TRUE > >> +INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> +!endif > >> + > >> > >> ##################################################################### > >> ########### > >> > >> [FV.FVMAIN_COMPACT] > >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index > >> 9dd6171..f04c36b 100644 > >> --- a/OvmfPkg/OvmfPkgX64.fdf > >> +++ b/OvmfPkg/OvmfPkgX64.fdf > >> @@ -355,6 +355,10 @@ [FV.DXEFV] > >> INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > >> INF OvmfPkg/PlatformDxe/Platform.inf > >> > >> +!if $(SMM_REQUIRE) == TRUE > >> +INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> +!endif > >> + > >> > >> ##################################################################### > >> ########### > >> > >> [FV.FVMAIN_COMPACT] > >> diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> new file mode 100644 > >> index 0000000..3ce48ae > >> --- /dev/null > >> +++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > >> @@ -0,0 +1,57 @@ > >> +## @file > >> +# A DXE_DRIVER providing SMRAM access by producing > >> EFI_SMM_ACCESS2_PROTOCOL. > >> +# > >> +# Q35 TSEG is expected to have been verified and set up by the > >> +SmmAccessPei # driver. > >> +# > >> +# Copyright (C) 2013, 2015, Red Hat, Inc. > >> +# > >> +# This program and the accompanying materials are licensed and made > >> +available # under the terms and conditions of the BSD License which > >> +accompanies this # distribution. The full text of the license may be > >> +found at # http://opensource.org/licenses/bsd-license.php > >> +# > >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > >> +BASIS, WITHOUT # WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > >> EXPRESS OR IMPLIED. > >> +# > >> +## > >> + > >> +[Defines] > >> + INF_VERSION = 0x00010005 > >> + BASE_NAME = SmmAccess2Dxe > >> + FILE_GUID = AC95AD3D-4366-44BF-9A62-E4B29D7A2206 > >> + MODULE_TYPE = DXE_DRIVER > >> + VERSION_STRING = 1.0 > >> + PI_SPECIFICATION_VERSION = 0x00010400 > >> + ENTRY_POINT = SmmAccess2DxeEntryPoint > >> + > >> +# > >> +# The following information is for reference only and not required by the > >> build tools. > >> +# > >> +# VALID_ARCHITECTURES = IA32 X64 > >> +# > >> + > >> +[Sources] > >> + SmmAccess2Dxe.c > >> + SmramInternal.c > > > > SmramInternal.h is missing from [Sources] section. > > It is not clear to me if there is a general requirement to list *.h files in > the [Sources] sections of INF files. I think we usually only list the > C files in OvmfPkg. > > $ git grep -l '\.h' -- 'OvmfPkg/*.inf' > > OvmfPkg/AcpiTables/AcpiTables.inf > OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf > OvmfPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf > OvmfPkg/SataControllerDxe/SataControllerDxe.inf > OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf > OvmfPkg/XenBusDxe/XenBusDxe.inf > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > > I vaguely remember that the last two were created with the UEFI driver > wizard. The others were all created by cloning and > customizing other > edk2 modules. So it seems to be confirmed that whenever we wrote a module > from scratch under OvmfPkg, we wouldn't list the > header files in [Sources], only the C, assembly and ASL files. > > I always thought the build utilities would automatically deduce the header > dependencies (recursively). > > Does this practice conflict with the edk2 specs? > > Anyway, I can fix this up. > > > > >> + > >> +[Packages] > >> + MdeModulePkg/MdeModulePkg.dec > >> + MdePkg/MdePkg.dec > >> + OvmfPkg/OvmfPkg.dec > >> + > >> +[LibraryClasses] > >> + DebugLib > >> + PcdLib > >> + PciLib > >> + UefiBootServicesTableLib > >> + UefiDriverEntryPoint > >> + > >> +[Protocols] > >> + gEfiSmmAccess2ProtocolGuid # PROTOCOL ALWAYS_PRODUCED > > > > Comment should be: ## PRODUCES > > I'll fix it up, but it's also unclear to me when ALWAYS_PRODUCED has become > deprecated. > > > > >> + > >> +[FeaturePcd] > >> + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > >> + > >> +[Depex] > >> + TRUE > >> diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c > >> b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c > >> new file mode 100644 > >> index 0000000..d513039 > >> --- /dev/null > >> +++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c > >> @@ -0,0 +1,156 @@ > >> +/** @file > >> + > >> + A DXE_DRIVER providing SMRAM access by producing > >> EFI_SMM_ACCESS2_PROTOCOL. > >> + > >> + Q35 TSEG is expected to have been verified and set up by the > >> + SmmAccessPei driver. > >> + > >> + Copyright (C) 2013, 2015, Red Hat, Inc.<BR> Copyright (c) 2009 - > >> + 2010, Intel Corporation. All rights reserved.<BR> > >> + > >> + This program and the accompanying materials are licensed and made > >> + available under the terms and conditions of the BSD License which > >> + accompanies this distribution. The full text of the license may be > >> + found at http://opensource.org/licenses/bsd-license.php > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > >> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > >> EXPRESS OR IMPLIED. > >> + > >> +**/ > >> + > >> +#include <Library/DebugLib.h> > >> +#include <Library/PcdLib.h> > >> +#include <Library/UefiBootServicesTableLib.h> > >> +#include <Protocol/SmmAccess2.h> > >> + > >> +#include "SmramInternal.h" > >> + > >> +/** > >> + Opens the SMRAM area to be accessible by a boot-service driver. > >> + > >> + This function "opens" SMRAM so that it is visible while not inside of > >> SMM. > >> + The function should return EFI_UNSUPPORTED if the hardware does > >> + not support hiding of SMRAM. The function should return > >> + EFI_DEVICE_ERROR if the SMRAM configuration is locked. > >> + > >> + @param[in] This The EFI_SMM_ACCESS2_PROTOCOL instance. > >> + > >> + @retval EFI_SUCCESS The operation was successful. > >> + @retval EFI_UNSUPPORTED The system does not support opening and > >> closing of > >> + SMRAM. > >> + @retval EFI_DEVICE_ERROR SMRAM cannot be opened, perhaps because it is > >> + locked. > >> +**/ > >> +STATIC > > > > Recommend removing STATIC from public functions. > > I understand that there are debuggability problems with some toolchains if > functions are declared static, but for OVMF we mostly use > manual instrumentation. (Or else, when people take the time to set up gdb > <https://edk2.bluestop.org/w/tianocore/debugging- > with-gdb/>, then gdb doesn't have issues with STATIC.) > > It is common C practice (and more closely, OvmfPkg practice) to keep protocol > member functions with internal ("static") linkage on the > language level, and expose them only via function pointers. I'd like to stick > with STATIC in OvmfPkg; we've never had problem reports > in this are. > > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccess2DxeOpen ( > >> + IN EFI_SMM_ACCESS2_PROTOCOL *This > >> + ) > >> +{ > >> + return SmramAccessOpen (&This->LockState, &This->OpenState); } > >> + > >> +/** > >> + Inhibits access to the SMRAM. > >> + > >> + This function "closes" SMRAM so that it is not visible while outside of > >> SMM. > >> + The function should return EFI_UNSUPPORTED if the hardware does > >> + not support hiding of SMRAM. > >> + > >> + @param[in] This The EFI_SMM_ACCESS2_PROTOCOL instance. > >> + > >> + @retval EFI_SUCCESS The operation was successful. > >> + @retval EFI_UNSUPPORTED The system does not support opening and > >> closing of > >> + SMRAM. > >> + @retval EFI_DEVICE_ERROR SMRAM cannot be closed. > >> +**/ > >> +STATIC > > > > Recommend removing STATIC from public functions. > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccess2DxeClose ( > >> + IN EFI_SMM_ACCESS2_PROTOCOL *This > >> + ) > >> +{ > >> + return SmramAccessClose (&This->LockState, &This->OpenState); } > >> + > >> +/** > >> + Inhibits access to the SMRAM. > >> + > >> + This function prohibits access to the SMRAM region. This function > >> + is usually implemented such that it is a write-once operation. > >> + > >> + @param[in] This The EFI_SMM_ACCESS2_PROTOCOL instance. > >> + > >> + @retval EFI_SUCCESS The device was successfully locked. > >> + @retval EFI_UNSUPPORTED The system does not support locking of SMRAM. > >> +**/ > >> +STATIC > > > > Recommend removing STATIC from public functions. > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccess2DxeLock ( > >> + IN EFI_SMM_ACCESS2_PROTOCOL *This > >> + ) > >> +{ > >> + return SmramAccessLock (&This->LockState, &This->OpenState); } > >> + > >> +/** > >> + Queries the memory controller for the possible regions that will > >> +support > >> + SMRAM. > >> + > >> + @param[in] This The EFI_SMM_ACCESS2_PROTOCOL instance. > >> + @param[in,out] SmramMapSize A pointer to the size, in bytes, of the > >> + SmramMemoryMap buffer. > >> + @param[in,out] SmramMap A pointer to the buffer in which firmware > >> + places the current memory map. > >> + > >> + @retval EFI_SUCCESS The chipset supported the given resource. > >> + @retval EFI_BUFFER_TOO_SMALL The SmramMap parameter was too small. The > >> + current buffer size needed to hold the > >> memory > >> + map is returned in SmramMapSize. > >> +**/ > >> +STATIC > > > > Recommend removing STATIC from public functions. > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccess2DxeGetCapabilities ( > >> + IN CONST EFI_SMM_ACCESS2_PROTOCOL *This, > >> + IN OUT UINTN *SmramMapSize, > >> + IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap > >> + ) > >> +{ > >> + return SmramAccessGetCapabilities (This->LockState, This->OpenState, > >> + SmramMapSize, SmramMap); > >> +} > >> + > >> +// > >> +// LockState and OpenState will be filled in by the entry point. > >> +// > >> +STATIC EFI_SMM_ACCESS2_PROTOCOL mAccess2 = { > >> + &SmmAccess2DxeOpen, > >> + &SmmAccess2DxeClose, > >> + &SmmAccess2DxeLock, > >> + &SmmAccess2DxeGetCapabilities > >> +}; > > > > Recommend removing STATIC from produced protocol structure. > > Global variable are typically placed before function implementation in a > > file. > > I placed the initialization of mAccess2 here because I wanted to avoid the > (quite verbose) forward declarations of the member > functions. > > Also, forward declarations make the "Jump To Tag" functionality of > ctags-compatible text editors quite inconvenient: the forward > declaration of the function gets highlighted instead of the function > definition. > > One way to avoid the forward declarations of the functions and to keep the > (initializer-less) definition of mAccess2 at the top would > be to explicitly assign the mAccess2.* fields in SmmAccess2DxeEntryPoint() > below. If you strongly dislike the code as-is, I can > implement this option. > > Thanks! > Laszlo > > > > >> + > >> +// > >> +// Entry point of this driver. > >> +// > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccess2DxeEntryPoint ( > >> + IN EFI_HANDLE ImageHandle, > >> + IN EFI_SYSTEM_TABLE *SystemTable > >> + ) > >> +{ > >> + // > >> + // This module should only be included if SMRAM support is required. > >> + // > >> + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > >> + > >> + GetStates (&mAccess2.LockState, &mAccess2.OpenState); > >> + return gBS->InstallMultipleProtocolInterfaces (&ImageHandle, > >> + &gEfiSmmAccess2ProtocolGuid, &mAccess2, > >> + NULL); > >> +} > >> -- > >> 1.8.3.1 > >> > >> > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
