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. [[email protected]: Use LocalApicLib to send trigger an SMI on all processors] Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Paolo Bonzini <[email protected]> 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
