CC'ing Mike and Jiewen On 10/13/15 12:32, Paolo Bonzini wrote: > > > On 09/10/2015 23:58, Laszlo Ersek wrote: >> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> new file mode 100644 >> index 0000000..e895fd6 >> --- /dev/null >> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> @@ -0,0 +1,365 @@ >> +/** @file >> + >> + A DXE_RUNTIME_DRIVER providing synchronous SMI activations via the >> + EFI_SMM_CONTROL2_PROTOCOL. >> + >> + We expect the PEI phase to have covered the following: >> + - ensure that the underlying QEMU machine type be Q35 >> + (responsible: OvmfPkg/SmmAccess/SmmAccessPei.inf) >> + - ensure that the ACPI PM IO space be configured >> + (responsible: OvmfPkg/PlatformPei/PlatformPei.inf) >> + >> + Our own entry point is responsible for confirming the SMI feature and for >> + configuring it. >> + >> + 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 <IndustryStandard/Q35MchIch9.h> >> +#include <Library/DebugLib.h> >> +#include <Library/IoLib.h> >> +#include <Library/PcdLib.h> >> +#include <Library/PciLib.h> >> +#include <Library/QemuFwCfgLib.h> >> +#include <Library/UefiBootServicesTableLib.h> >> +#include <Protocol/S3SaveState.h> >> +#include <Protocol/SmmControl2.h> >> + >> +// >> +// The absolute IO port address of the SMI Control and Enable Register. It >> is >> +// only used to carry information from the entry point function to the >> +// S3SaveState protocol installation callback, strictly before the runtime >> +// phase. >> +// >> +STATIC UINTN mSmiEnable; >> + >> +// >> +// Event signaled when an S3SaveState protocol interface is installed. >> +// >> +STATIC EFI_EVENT mS3SaveStateInstalled; >> + >> +/** >> + Invokes SMI activation from either the preboot or runtime environment. >> + >> + This function generates an SMI. >> + >> + @param[in] This The EFI_SMM_CONTROL2_PROTOCOL instance. >> + @param[in,out] CommandPort The value written to the command port. >> + @param[in,out] DataPort The value written to the data port. >> + @param[in] Periodic Optional mechanism to engender a >> periodic >> + stream. >> + @param[in] ActivationInterval Optional parameter to repeat at this >> + period one time or, if the Periodic >> + Boolean is set, periodically. >> + >> + @retval EFI_SUCCESS The SMI/PMI has been engendered. >> + @retval EFI_DEVICE_ERROR The timing is unsupported. >> + @retval EFI_INVALID_PARAMETER The activation period is unsupported. >> + @retval EFI_INVALID_PARAMETER The last periodic activation has not been >> + cleared. >> + @retval EFI_NOT_STARTED The SMM base service has not been >> initialized. >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +SmmControl2DxeTrigger ( >> + IN CONST EFI_SMM_CONTROL2_PROTOCOL *This, >> + IN OUT UINT8 *CommandPort OPTIONAL, >> + IN OUT UINT8 *DataPort OPTIONAL, >> + IN BOOLEAN Periodic OPTIONAL, >> + IN UINTN ActivationInterval OPTIONAL >> + ) >> +{ >> + // >> + // No support for queued or periodic activation. >> + // >> + if (Periodic || ActivationInterval > 0) { >> + return EFI_DEVICE_ERROR; >> + } >> + >> + // >> + // The so-called "Advanced Power Management Status Port Register" is in >> fact >> + // a generic data passing register, between the caller and the SMI >> + // dispatcher. The ICH9 spec calls it "scratchpad register" -- calling it >> + // "status" elsewhere seems quite the misnomer. Status registers usually >> + // report about hardware status, while this register is fully governed by >> + // software. >> + // >> + // Write to the status register first, as this won't trigger the SMI just >> + // yet. Then write to the control register. >> + // >> + IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); >> + IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort); >> + return EFI_SUCCESS; >> +} > > I tested multiprocessor SMM with your branch, and we're really close. > KVM doesn't handle SMIs correctly (ironically I fixed the same bug in > QEMU with commit a9bad65, "target-i386: wake up processors that > receive an SMI", 2015-05-19). TCG boots... but it takes about 5 > minutes. > > This is because the SMI handler, and in particular > SmmWaitForApArrival, expects that SmmControl2DxeTrigger triggers an > SMI IPI on all processors rather than just the BSP. QEMU's port 0xb2 > only affects the current CPU, thus all SMM invocations loop for a > second (the default value of PcdCpuSmmApSyncTimeout) before > SmmWaitForApArrival sends another SMI IPI to the APs. > > This could also be fixed by setting > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode to 1 (aka > SmmCpuSyncModeRelaxedAp), but this only takes effect on X64 CPUs which > also ironically we don't support yet. Therefore, the following seems > to be the correct fix. With it, TCG still takes a while to boot, but > KVM gets to the UEFI shell very fast. > > [pbonz...@redhat.com: Use LocalApicLib to send trigger an SMI on all > processors] > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > index e895fd6..17fbb88 100644 > --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > @@ -28,6 +28,7 @@ > #include <IndustryStandard/Q35MchIch9.h> > #include <Library/DebugLib.h> > #include <Library/IoLib.h> > +#include <Library/LocalApicLib.h> > #include <Library/PcdLib.h> > #include <Library/PciLib.h> > #include <Library/QemuFwCfgLib.h> > @@ -88,6 +89,15 @@ SmmControl2DxeTrigger ( > } > > // > + // The write to the control register is synchronous and only affects the > + // current CPU, so bring in the APs first. The SMI handler expects that > + // all APs will rendez-vous within one PcdCpuSmmApSyncTimeout (though it > + // helpfully tries sending SMI IPIs to the missing processors if there are > + // any). > + // > + SendSmiIpiAllExcludingSelf (); > + > + // > // The so-called "Advanced Power Management Status Port Register" is in > fact > // a generic data passing register, between the caller and the SMI > // dispatcher. The ICH9 spec calls it "scratchpad register" -- calling it > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > index bc66a27..f9c4821 100644 > --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > @@ -44,10 +44,12 @@ > [Packages] > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > > [LibraryClasses] > DebugLib > IoLib > + LocalApicLib > PcdLib > PciLib > QemuFwCfgLib >
Thank you. This looks very effective, and it's also easy to squash into my patch (with the right attribution of course). However, it raises a number of interesting questions. First of all, if the edk2 reference code (in the SMM core and in PiSmmCpuDxeSmm) depends on such behavior justifiedly, then I think we have a bug in the PI specification. Namely, version 1.4 thereof does not seem to require that EFI_SMM_CONTROL2_PROTOCOL.Trigger() raise an SMI on *all* processors. (See volume 4, section 5.4.) In particular, the EFI_SMM_CONTROL2_PROTOCOL.Trigger() specification makes many references to SMI handling and dispatching. But I cannot make a mental connection between an SMI "broadcast", and "handling and dispatching" in edk2, since all "handling and dispatching" code in edk2 that is exposed via protocols, is non-reentrant with regard to multiple VCPUs, to my knowledge. One could argue that whatever code handles the SMI on the BP is responsible for bringing the APs into SMM as well, before doing any sensitive work. I'm not sure. Second, if writing to ioport 0xb2 should *automatically* raise an SMI on all processors, then the QEMU code could be incorrect. However I could never derive such an "imperative" from the ICH9 spec. Third, we can look at the implementation of EFI_SMM_CONTROL2_PROTOCOL in the Quark package. It is in the file QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmControlDxe/SmmControlDriver.c and the relevant function is called Activate(). ( For easy sharing, I just googled the filename, hoping for at least one hit, and yes there is one: https://kernel.googlesource.com/pub/scm/linux/kernel/git/jejb/Quark_EDKII/+/master/QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmControlDxe/SmmControlDriver.c ) It is very similar to my code (although any similarity is purely unintended); it doesn't seem to send SMI IPIs to APs. On the other hand, I don't know if Quark is a UP vs. MP system to begin with. So, I think we're missing something in the design, and in the "interplay" between the PI spec and the edk2 reference code (SMM core, PiSmmCpuDxeSmm). Again, I'd be very glad (and thankful) to pick up this hunk (and to give credit), but first I'd like to understand why we have to resort to this, given that my implementation (without this fixup) *should* already conform to the PI spec. Let's look at the context where EFI_SMM_CONTROL2_PROTOCOL.Trigger() is invoked. First, the setup. When PiSmmCpuSmmDxe loads, it installs the EFI_SMM_CONFIGURATION_PROTOCOL interface. This happens in PiCpuSmmEntry(), file "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c". The one member function exposed by that interface is RegisterSmmEntry(). Now, when this protocol iface is installed, the SMM core gets a protocol notify callback -- see the SmmIplSmmConfigurationEventNotify() function in "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c" -- and the SMM core calls the RegisterSmmEntry() function of PiSmmCpuSmmDxe right away. The function pointer argument that is passed in comes from SmmMain() [MdeModulePkg/Core/PiSmmCore/PiSmmCore.c], and it points to SmmEntryPoint() [same file]. This in turn will result in PiSmmCpuSmmDxe stashing a pointer to the SmmEntryPoint() function of the SMM core in "gSmmCpuPrivate->SmmCoreEntry". See the RegisterSmmEntry() function in "UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c". So, for the setup, we can come up with the following call trees: - When the SMM core launches: SmmMain() [MdeModulePkg/Core/PiSmmCore/PiSmmCore.c] gSmmCorePrivate->SmmEntryPoint = SmmEntryPoint - When PiSmmCpuDxeSmm is loaded into SMRAM and started: PiCpuSmmEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] InstallMultipleProtocolInterfaces() [...] SmmIplSmmConfigurationEventNotify() [MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c] RegisterSmmEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] gSmmCpuPrivate->SmmCoreEntry = SmmEntryPoint Second, the handling and dispatching. The only location that matters for us is the EFI_SMM_CONTROL2_PROTOCOL.Trigger() call in "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c", the SmmCommunicationCommunicate() function, which is the implementation of the EFI_SMM_COMMUNICATION_PROTOCOL.Communicate() member. The sole purpose of this function is to "call into SMM" with a preformatted "communication buffer" (a message) from the outside. All the drivers that are split into non-priv and priv halves rely on this service to call their priv halves. So, assuming a driver wishes to make such a privileged call, it ends up in SmmCommunicationCommunicate(). From then onwards: SmmCommunicationCommunicate() [MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c] SmmControl2DxeTrigger() [OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c] // // by way of SMI -- privileged from here on // _SmiEntryPoint() [UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S] SmiRendezvous() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] BSPHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] SmmEntryPoint() [MdeModulePkg/Core/PiSmmCore/PiSmmCore.c] SmiManage() [MdeModulePkg/Core/PiSmmCore/Smi.c] // // dispatch // APHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] ... But, you are completely right. SmiRendezvous() expects to be executed concurrently, by several processors. I just don't have the slightest clue why the APs would *be there*. So, something is missing. Either the EFI_SMM_CONTROL2_PROTOCOL.Trigger() spec is incomplete in PI, or QEMU's emulation of APM_CNT is not appropriate. Either way, I'm okay with this fix, but I'd like to understand the expectations. Mike, Jiewen, can you please help investigate this? Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
