On 11/23/15 21:28, Kinney, Michael D wrote: > 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]
Ah! I didn't know that. I though that such dependencies would be deduced automatically, recursively. Thank you for the info. In the future I'll list header files too. > 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. Hm. OvmfPkg is not part of UDK, so it probably should not be a subject to UPT rules. But, that's the less relevant remark here. The more relevant would be: we occasionally have files in a module subdirectory that are not processable with the build tools, yet if the module was distributed (in any format), those files should be included. For example, a helper shell script that is not necessary for every build, but useful for preparing changes. Not adding it to [Sources] would break packaging (any kind of packaging), whereas adding it to [Sources] would cause the build to fail. I think for now we should certainly include the .h files, and keep the existing practice for non-source files. > Use of STATIC in OvmfPkg is ok. I only mentioned it to be consistent with > other packages. Thank you -- that's good to be reminded of; occasionally I might write code for AppPkg or Mde[Module]Pkg. > > 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. I'd certainly like to find a compromise that sticks with the style of the other packages, while allowing me to use ctags without much inconvenience. :) Thanks Laszlo > > 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