Ersek, Thanks your details description. Yes. I suggest to hold on this check-in till SMM-capable S3 solution on OVMF is finalized and more stable.
Best Regards, Jeff -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Monday, May 11, 2015 6:22 PM To: Fan, Jeff; edk2-devel@lists.sourceforge.net Cc: Chen Fan; Justen, Jordan L; Zimmer, Vincent Subject: Re: [PATCH 08/11] UefiCpuPkg: CpuDxe: optionally save MTRR settings to AcpiNVS memory block On 05/11/15 11:04, Fan, Jeff wrote: > 1. PCD PcdCpuMtrrTableAddress/PcdCpuSyncMtrrToAcpiNvs are not > necessary now, due to there is no one total SMM-capable S3 resume. I > think you could add them (if necessary) after one total SMM-capable > S3 resume is verified. The problem is that this leads to a circular dependency, to some extent. :) Namely, the SMM-capable S3 stuff is something I'm currently working on *for OVMF*, both by writing new code and by porting (the most critical modules) from Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg. If we don't add this feature-let to UefiCpuPkg, then the stuff I'm working on will not function, and the total SMM-capable S3 resume won't be possible to verify. ( Some background: in Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg, three (or four) drivers work together on the MTRR settings. First, as I wrote in the commit message for patch 8, CpuArchDxe reflects the full set of MTRR settings to the AcpiNVS object in question whenever EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() is called. Second, CpuMpDxe *links* (not copies) this AcpiNVS object with the MTRR settings into the ACPI_CPU_DATA.MtrrTable field, when PiSmmCpuDxeSmm installs the EFI_SMM_CONFIGURATION_PROTOCOL, during normal boot. Third, PiSmmCpuDxeSmm grabs the most recent MTRR settings from the AcpiNVS object in question and stashes them in SMRAM when EFI_SMM_READY_TO_LOCK_PROTOCOL is emitted by the SMM core. Fourth... during S3 resume, the S3 Resume PEIM transfers control to the SMRAM-resident image of PiSmmCpuDxeSmm temporarily, and then PiSmmCpuDxeSmm restores the MTRR settings from SMRAM. ) So... Are you suggesting that I hold on to this patch until I have something that works end-to-end, and then post this patch as part of *that* series? That was actually my original intent, but the series in question is becoming quite large, and it will be hard to find reviewers for it if I patch-bomb the list with it in one go. That's why I'm trying to flush these patches early. But, if you say this is premature for UefiCpuPkg at the moment, I can hold on to it. However, in that case, I think I should delay patches 9 and 10 too. Alternatively, I could create a version of patches 8 through 10 that do the MTRR broadcast *without* the AcpiNVS object. Unfortunately, that's not in my interest; that just creates *more* work for me -- I'd both have to send patches for UefiCpuPkg now *and* update them later (with the AcpiNVS object and the PCDs) when posting the larger SMM set. So I guess if patches 8 through 10 are not appropriate for UefiCpuPkg in this form right now, then I'll just hang on to them in my branch until later. > > 2. The patch introduced two new files with the different BSD license > header format from the other files in the same Package. I do not > suggest to use the different BSD license header here. I don't have any choice in this matter -- only Intel has. The files in question are almost verbatim copies (therefore, definitely derivative works of): Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe/MtrrSync.h Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe/MtrrSync.c and those are covered by Intel's copyright. I can not change the copyright terms (3-clause BSDL) set forth by the copyright holder. There are four options: (a) If Intel is willing to relicense these files under the 2-clause BSDL (which most files in UefiCpuPkg are under, apparently), then everything should be okay. (b) Otherwise, you could accept these files with their original 3-clause BSDL licenses; it is permitted by "UefiCpuPkg/Contributions.txt". (c) None of the above works -- in this case we'll have to clone UefiCpuPkg/CpuDxe under OvmfPkg, and (maybe?) put all of that CpuDxe clone under the 3-clause BSDL. That would be a horrible outcome. (d) Finally, I can eliminate MTRR handling from my SMM work (see the background above), ripping out chunks of code from the CpuMpDxe port and the PiSmmCpuDxeSmm port. (Note, I'm reconstructing CpuMpDxe under a different name in OvmfPkg, with only the ACPI_CPU_DATA / S3-related functionality anyway, but MtrrTable *does* belong there.) Removing MTRR handling would be arguably the simplest, but (arguably) the least correct thing to do as well. Thanks Laszlo > Jordan, > Any comments on new files' license header? > > Thanks! > Jeff > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Saturday, May 09, 2015 2:52 AM > To: edk2-devel@lists.sourceforge.net > Cc: Fan, Jeff; Chen Fan; Justen, Jordan L > Subject: [PATCH 08/11] UefiCpuPkg: CpuDxe: optionally save MTRR > settings to AcpiNVS memory block > > The > > Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe > > driver provides the following capability in its implementation of > EFI_CPU_ARCH_PROTOCOL: whenever the SetMemoryAttributes() member is used > (directly, or indirectly via gDS->SetMemorySpaceAttributes()) to change MTRR > settings, the complete set of settings is immediately reflected to an AcpiNVS > memory block. > > (The address of said block is published via a dynamic PCD.) > > This feature is necessary for supporting SMM-capable S3 resume, hence port it > to UefiCpuPkg. In IA32FamilyCpuBasePkg the feature is always built in; here > we make it controllable with a feature PCD. > > Cc: Jeff Fan <jeff....@intel.com> > Cc: Chen Fan <chen.fan.f...@cn.fujitsu.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > UefiCpuPkg/CpuDxe/CpuDxe.inf | 6 + > UefiCpuPkg/CpuDxe/MtrrSync.h | 86 ++++++++++++++ > UefiCpuPkg/CpuDxe/CpuDxe.c | 17 +++ > UefiCpuPkg/CpuDxe/MtrrSync.c | 118 ++++++++++++++++++++ > UefiCpuPkg/UefiCpuPkg.dec | 11 ++ > 5 files changed, 238 insertions(+) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf > b/UefiCpuPkg/CpuDxe/CpuDxe.inf index 61bc55a..35e48e1 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf > @@ -52,6 +52,8 @@ > CpuGdt.h > CpuMp.c > CpuMp.h > + MtrrSync.c > + MtrrSync.h > > [Sources.IA32] > Ia32/CpuAsm.asm | MSFT > @@ -80,6 +82,10 @@ > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMtrrTableAddress ## > SOMETIMES_PRODUCES > + > +[FeaturePcd] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSyncMtrrToAcpiNvs > > [Depex] > TRUE > diff --git a/UefiCpuPkg/CpuDxe/MtrrSync.h > b/UefiCpuPkg/CpuDxe/MtrrSync.h new file mode 100644 index > 0000000..1c20a32 > --- /dev/null > +++ b/UefiCpuPkg/CpuDxe/MtrrSync.h > @@ -0,0 +1,86 @@ > +/** @file > + > + Include file for MTRR synchronzation. > + > + Copyright (C) 2015, Red Hat, Inc. > + Copyright (c) 2013-2015 Intel Corporation. > + > + Redistribution and use in source and binary forms, with or without > + modification, are permitted provided that the following conditions > + are met: > + > + * Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > + * Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + * Neither the name of Intel Corporation nor the names of its > + contributors may be used to endorse or promote products derived > + from this software without specific prior written permission. > + > + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > + FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > + COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > + INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, > + BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > + CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN > + ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY > OF SUCH DAMAGE. > + > + Module Name: MtrrSync.h > + > +**/ > + > +#ifndef _EFI_MTRR_SYNC_H_ > +#define _EFI_MTRR_SYNC_H_ > + > +#include <Library/MtrrLib.h> > + > +extern MTRR_SETTINGS *mMtrrTable; > + > +/** > + Initialize memory region for MTRR data. > + > + This function allocates ACPI NVS memory for MTRR data, and fills > + the region with current MTRR data. Each time MTRRs are written, > + this memory region will be updated accordingly. > + > +**/ > +VOID > +InitializeMtrrData ( > + VOID > + ); > + > +/** > + Synchronzies up the MTRR values with BSP for calling processor. > + > + This function synchronzies up the MTRR values with BSP for calling > processor. > + > + @param Buffer Mtrr table address. > + > +**/ > +VOID > +EFIAPI > +LoadMtrrData ( > + VOID *Buffer > + ); > + > +/** > + Allocate EfiACPIMemoryNVS below 4G memory address. > + > + This function allocates EfiACPIMemoryNVS below 4G memory address. > + > + @param Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +VOID* > +AllocateAcpiNvsMemoryBelow4G ( > + IN UINTN Size > + ); > +#endif > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c > index c9df4e1..3d11807 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -14,6 +14,7 @@ > > #include "CpuDxe.h" > #include "CpuMp.h" > +#include "MtrrSync.h" > > // > // Global Variables > @@ -405,6 +406,14 @@ CpuSetMemoryAttributes ( > CacheType > ); > > + if (!RETURN_ERROR (Status)) { > + if (FeaturePcdGet (PcdCpuSyncMtrrToAcpiNvs)) { > + // > + // Sync saved MTRR settings > + // > + MtrrGetAllMtrrs (mMtrrTable); > + } > + } > return (EFI_STATUS) Status; > } > > @@ -870,6 +879,14 @@ InitializeCpu ( > // > ProgramVirtualWireMode (); > > + if (FeaturePcdGet (PcdCpuSyncMtrrToAcpiNvs)) { > + // > + // Allocates ACPI NVS memory for MTRR data. > + // > + InitializeMtrrData (); > + MtrrGetAllMtrrs (mMtrrTable); > + } > + > // > // Install CPU Architectural Protocol > // > diff --git a/UefiCpuPkg/CpuDxe/MtrrSync.c > b/UefiCpuPkg/CpuDxe/MtrrSync.c new file mode 100644 index > 0000000..8538014 > --- /dev/null > +++ b/UefiCpuPkg/CpuDxe/MtrrSync.c > @@ -0,0 +1,118 @@ > +/** @file > + > + Code for MTRR synchronzation. > + > + Copyright (C) 2015, Red Hat, Inc. > + Copyright (c) 2013-2015 Intel Corporation. > + > + Redistribution and use in source and binary forms, with or without > + modification, are permitted provided that the following conditions > + are met: > + > + * Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > + * Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + * Neither the name of Intel Corporation nor the names of its > + contributors may be used to endorse or promote products derived > + from this software without specific prior written permission. > + > + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > + FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > + COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > + INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, > + BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > + CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN > + ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY > OF SUCH DAMAGE. > + > + Module Name: MtrrSync.c > + > +**/ > + > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/DebugLib.h> > +#include <Library/BaseMemoryLib.h> > + > +#include "MtrrSync.h" > + > +MTRR_SETTINGS *mMtrrTable; > + > +/** > + Initialize memory region for MTRR data. > + > + This function allocates ACPI NVS memory for MTRR data. > + Each time MTRRs are written, this memory region will be updated > accordingly. > + > +**/ > +VOID > +InitializeMtrrData ( > + VOID > + ) > +{ > + // > + // Allocate memory for fixed MTRRs, variable MTRRs and > +MTRR_DEF_TYPE > + // > + mMtrrTable = AllocateAcpiNvsMemoryBelow4G (sizeof (MTRR_SETTINGS)); > + > + PcdSet64 (PcdCpuMtrrTableAddress, (UINT64) (UINTN) mMtrrTable); } > + > +/** > + Synchronzies up the MTRR values with BSP for calling processor. > + > + This function synchronzies up the MTRR values with BSP for calling > processor. > + > + @param Buffer Mtrr table address. > + > +**/ > +VOID > +EFIAPI > +LoadMtrrData ( > + VOID *Buffer > + ) > +{ > + MtrrSetAllMtrrs (Buffer); > +} > + > +/** > + Allocate EfiACPIMemoryNVS below 4G memory address. > + > + This function allocates EfiACPIMemoryNVS below 4G memory address. > + > + @param Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +VOID* > +AllocateAcpiNvsMemoryBelow4G ( > + IN UINTN Size > + ) > +{ > + UINTN Pages; > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > + VOID* Buffer; > + > + Pages = EFI_SIZE_TO_PAGES (Size); > + Address = 0xffffffff; > + > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + Pages, > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + > + Buffer = (VOID *) (UINTN) Address; > + ZeroMem (Buffer, Size); > + > + return Buffer; > +} > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index 14a0bf2..5297c3b 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -62,5 +62,16 @@ > # @Prompt Configure stack size for Application Processor (AP) > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize|0x8000|UINT32|0x00000003 > > +[PcdsFeatureFlag] > + ## This flag controls whether the EFI_CPU_ARCH_PROTOCOL > +implementation > + # reflects the updated MTRR settings to a persistent (AcpiNVS) memory > block. > + # @Prompt Keep MTRR settings reflected to an AcpiNVS memory block > + > +gUefiCpuPkgTokenSpaceGuid.PcdCpuSyncMtrrToAcpiNvs|FALSE|BOOLEAN|0x000 > +00 > +004 > + > +[PcdsDynamic, PcdsDynamicEx] > + ## If PcdCpuSyncMtrrToAcpiNvs is TRUE, the PCD below will be set to > +the > + # address of the AcpiNVS memory block in question. > + > +gUefiCpuPkgTokenSpaceGuid.PcdCpuMtrrTableAddress|0x0|UINT64|0x0000000 > +5 > + > [UserExtensions.TianoCore."ExtraFiles"] > UefiCpuPkgExtra.uni > -- > 1.8.3.1 > > ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
