Laszlo, The section [UserExtensions.TianoCore."ExtraFiles"] can be used to list files not required to build and support incremental builds, but need to be included in a UDP package.
Mike > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, November 23, 2015 12:43 PM > 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/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
