* [PATCH v2 0/4] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode @ 2016-11-18 13:52 Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-18 13:52 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan, Jordan Justen, Michael Kinney, Paolo Bonzini This is version 2 of the series originally posted here: <https://lists.01.org/pipermail/edk2-devel/2016-November/004702.html>. In this version, OVMF negotiates the broadcast SMI feature with QEMU dynamically, using the interface added in this QEMU series: <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>. If the feature is available, OVMF selects it, and also re-sets the UefiCpuPkg PCDs that are related to BSP-AP synchronization to their "UefiCpuPkg.dec" defaults. Repo: https://github.com/lersek/edk2/ Branch: broadcast_smi_v2 BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Cc: Jeff Fan <jeff.fan@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Thanks Laszlo Laszlo Ersek (4): UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode OvmfPkg/IndustryStandard: add macros for QEMU's SMI feature control bits OvmfPkg/SmmControl2Dxe: select broadcast SMI if available OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 9 ++- OvmfPkg/OvmfPkgIa32.dsc | 7 +- OvmfPkg/OvmfPkgIa32X64.dsc | 7 +- OvmfPkg/OvmfPkgX64.dsc | 7 +- OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 73 +++++++++++++++++++- OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 5 ++ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 4 +- UefiCpuPkg/UefiCpuPkg.dec | 20 +++--- 8 files changed, 110 insertions(+), 22 deletions(-) -- 2.9.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode 2016-11-18 13:52 [PATCH v2 0/4] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek @ 2016-11-18 13:52 ` Laszlo Ersek 2016-11-22 7:20 ` Fan, Jeff 2016-11-18 13:52 ` [PATCH v2 2/4] OvmfPkg: dynamic defaults for " Laszlo Ersek ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2016-11-18 13:52 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan, Jordan Justen, Michael Kinney, Paolo Bonzini Move the declaration of these PCDs from the [PcdsFixedAtBuild, PcdsPatchableInModule] section of "UefiCpuPkg/UefiCpuPkg.dec" to the [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] section. Their types, default values, and token values remain unchanged. Only UefiCpuPkg/PiSmmCpuDxeSmm consumes these PCDs, specifically on the call stack of its entry point function, and it turns them into static or dynamically allocated data in SMRAM: PiCpuSmmEntry() [PiSmmCpuDxeSmm.c] InitializeSmmTimer() [SyncTimer.c] PcdCpuSmmApSyncTimeout -> mTimeoutTicker InitializeMpServiceData() [MpService.c] InitializeMpSyncData() [MpService.c] PcdCpuSmmSyncMode -> mSmmMpSyncData->EffectiveSyncMode However, there's another call path to fetching "PcdCpuSmmSyncMode", namely SmmInitHandler() [PiSmmCpuDxeSmm.c] InitializeMpSyncData() [MpService.c] PcdCpuSmmSyncMode -> mSmmMpSyncData->EffectiveSyncMode and this path is exercised during S3 resume (as stated by the comment in SmmInitHandler() too, "Initialize private data during S3 resume"). While we can call the PCD protocol (via PcdLib) for fetching dynamic PCDs in the entry point function, we cannot do that at S3 resume. Therefore pre-fetch PcdCpuSmmSyncMode into a new global variable (which lives in SMRAM) in InitializeMpServiceData(), just before calling InitializeMpSyncData(). This way InitializeMpSyncData() can retrieve the stashed PCD value from SMRAM, regardless of the boot mode. Cc: Jeff Fan <jeff.fan@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new in v2 UefiCpuPkg/UefiCpuPkg.dec | 20 ++++++++++---------- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 4 +++- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index 89779c447d50..ca560398bbef 100644 --- a/UefiCpuPkg/UefiCpuPkg.dec +++ b/UefiCpuPkg/UefiCpuPkg.dec @@ -156,10 +156,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] # @Prompt Processor stack size in SMM. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x2000|UINT32|0x32132105 - ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. - # @Prompt AP synchronization timeout value in SMM. - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104 - ## Indicates if SMM Code Access Check is enabled. # If enabled, the SMM handler cannot execute the code outside SMM regions. # This PCD is suggested to TRUE in production image.<BR><BR> @@ -168,12 +164,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] # @Prompt SMM Code Access Check. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable|TRUE|BOOLEAN|0x60000013 - ## Indicates the CPU synchronization method used when processing an SMI. - # 0x00 - Traditional CPU synchronization method.<BR> - # 0x01 - Relaxed CPU synchronization method.<BR> - # @Prompt SMM CPU Synchronization Method. - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 - ## Specifies the number of variable MTRRs reserved for OS use. The default number of # MTRRs reserved for OS use is 2. # @Prompt Number of reserved variable MTRRs. @@ -214,6 +204,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] # @Prompt Use static page table for all memory in SMM. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D + ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. + # @Prompt AP synchronization timeout value in SMM. + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104 + + ## Indicates the CPU synchronization method used when processing an SMI. + # 0x00 - Traditional CPU synchronization method.<BR> + # 0x01 - Relaxed CPU synchronization method.<BR> + # @Prompt SMM CPU Synchronization Method. + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 + [PcdsDynamic, PcdsDynamicEx] ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA. # @Prompt The pointer to a CPU S3 data buffer. diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index 9b8db90ff6ed..cfbf59e8f2ca 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -24,6 +24,7 @@ UINTN mSmmMpSyncDataSize; SMM_CPU_SEMAPHORES mSmmCpuSemaphores; UINTN mSemaphoreSize; SPIN_LOCK *mPFLock = NULL; +SMM_CPU_SYNC_MODE mCpuSmmSyncMode; /** Performs an atomic compare exchange operation to get semaphore. @@ -1338,7 +1339,7 @@ InitializeMpSyncData ( // mSmmMpSyncData->BspIndex = (UINT32)-1; } - mSmmMpSyncData->EffectiveSyncMode = (SMM_CPU_SYNC_MODE) PcdGet8 (PcdCpuSmmSyncMode); + mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode; mSmmMpSyncData->Counter = mSmmCpuSemaphores.SemaphoreGlobal.Counter; mSmmMpSyncData->InsideSmm = mSmmCpuSemaphores.SemaphoreGlobal.InsideSmm; @@ -1392,6 +1393,7 @@ InitializeMpServiceData ( (sizeof (SMM_CPU_DATA_BLOCK) + sizeof (BOOLEAN)) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; mSmmMpSyncData = (SMM_DISPATCHER_MP_SYNC_DATA*) AllocatePages (EFI_SIZE_TO_PAGES (mSmmMpSyncDataSize)); ASSERT (mSmmMpSyncData != NULL); + mCpuSmmSyncMode = (SMM_CPU_SYNC_MODE)PcdGet8 (PcdCpuSmmSyncMode); InitializeMpSyncData (); // -- 2.9.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode 2016-11-18 13:52 ` [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek @ 2016-11-22 7:20 ` Fan, Jeff 2016-11-22 8:06 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Fan, Jeff @ 2016-11-22 7:20 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Justen, Jordan L, Kinney, Michael D, Paolo Bonzini Reviewed-by: Jeff Fan <jeff.fan@intel.com> -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Friday, November 18, 2016 9:53 PM To: edk2-devel-01 Cc: Fan, Jeff; Justen, Jordan L; Kinney, Michael D; Paolo Bonzini Subject: [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Move the declaration of these PCDs from the [PcdsFixedAtBuild, PcdsPatchableInModule] section of "UefiCpuPkg/UefiCpuPkg.dec" to the [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] section. Their types, default values, and token values remain unchanged. Only UefiCpuPkg/PiSmmCpuDxeSmm consumes these PCDs, specifically on the call stack of its entry point function, and it turns them into static or dynamically allocated data in SMRAM: PiCpuSmmEntry() [PiSmmCpuDxeSmm.c] InitializeSmmTimer() [SyncTimer.c] PcdCpuSmmApSyncTimeout -> mTimeoutTicker InitializeMpServiceData() [MpService.c] InitializeMpSyncData() [MpService.c] PcdCpuSmmSyncMode -> mSmmMpSyncData->EffectiveSyncMode However, there's another call path to fetching "PcdCpuSmmSyncMode", namely SmmInitHandler() [PiSmmCpuDxeSmm.c] InitializeMpSyncData() [MpService.c] PcdCpuSmmSyncMode -> mSmmMpSyncData->EffectiveSyncMode and this path is exercised during S3 resume (as stated by the comment in SmmInitHandler() too, "Initialize private data during S3 resume"). While we can call the PCD protocol (via PcdLib) for fetching dynamic PCDs in the entry point function, we cannot do that at S3 resume. Therefore pre-fetch PcdCpuSmmSyncMode into a new global variable (which lives in SMRAM) in InitializeMpServiceData(), just before calling InitializeMpSyncData(). This way InitializeMpSyncData() can retrieve the stashed PCD value from SMRAM, regardless of the boot mode. Cc: Jeff Fan <jeff.fan@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new in v2 UefiCpuPkg/UefiCpuPkg.dec | 20 ++++++++++---------- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 4 +++- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index 89779c447d50..ca560398bbef 100644 --- a/UefiCpuPkg/UefiCpuPkg.dec +++ b/UefiCpuPkg/UefiCpuPkg.dec @@ -156,10 +156,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] # @Prompt Processor stack size in SMM. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x2000|UINT32|0x32132105 - ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. - # @Prompt AP synchronization timeout value in SMM. - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104 - ## Indicates if SMM Code Access Check is enabled. # If enabled, the SMM handler cannot execute the code outside SMM regions. # This PCD is suggested to TRUE in production image.<BR><BR> @@ -168,12 +164,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] # @Prompt SMM Code Access Check. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable|TRUE|BOOLEAN|0x60000013 - ## Indicates the CPU synchronization method used when processing an SMI. - # 0x00 - Traditional CPU synchronization method.<BR> - # 0x01 - Relaxed CPU synchronization method.<BR> - # @Prompt SMM CPU Synchronization Method. - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 - ## Specifies the number of variable MTRRs reserved for OS use. The default number of # MTRRs reserved for OS use is 2. # @Prompt Number of reserved variable MTRRs. @@ -214,6 +204,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] # @Prompt Use static page table for all memory in SMM. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D + ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. + # @Prompt AP synchronization timeout value in SMM. + + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x3213 + 2104 + + ## Indicates the CPU synchronization method used when processing an SMI. + # 0x00 - Traditional CPU synchronization method.<BR> + # 0x01 - Relaxed CPU synchronization method.<BR> + # @Prompt SMM CPU Synchronization Method. + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 + [PcdsDynamic, PcdsDynamicEx] ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA. # @Prompt The pointer to a CPU S3 data buffer. diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index 9b8db90ff6ed..cfbf59e8f2ca 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -24,6 +24,7 @@ UINTN mSmmMpSyncDataSize; SMM_CPU_SEMAPHORES mSmmCpuSemaphores; UINTN mSemaphoreSize; SPIN_LOCK *mPFLock = NULL; +SMM_CPU_SYNC_MODE mCpuSmmSyncMode; /** Performs an atomic compare exchange operation to get semaphore. @@ -1338,7 +1339,7 @@ InitializeMpSyncData ( // mSmmMpSyncData->BspIndex = (UINT32)-1; } - mSmmMpSyncData->EffectiveSyncMode = (SMM_CPU_SYNC_MODE) PcdGet8 (PcdCpuSmmSyncMode); + mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode; mSmmMpSyncData->Counter = mSmmCpuSemaphores.SemaphoreGlobal.Counter; mSmmMpSyncData->InsideSmm = mSmmCpuSemaphores.SemaphoreGlobal.InsideSmm; @@ -1392,6 +1393,7 @@ InitializeMpServiceData ( (sizeof (SMM_CPU_DATA_BLOCK) + sizeof (BOOLEAN)) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; mSmmMpSyncData = (SMM_DISPATCHER_MP_SYNC_DATA*) AllocatePages (EFI_SIZE_TO_PAGES (mSmmMpSyncDataSize)); ASSERT (mSmmMpSyncData != NULL); + mCpuSmmSyncMode = (SMM_CPU_SYNC_MODE)PcdGet8 (PcdCpuSmmSyncMode); InitializeMpSyncData (); // -- 2.9.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode 2016-11-22 7:20 ` Fan, Jeff @ 2016-11-22 8:06 ` Laszlo Ersek 2016-11-22 8:09 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2016-11-22 8:06 UTC (permalink / raw) To: Fan, Jeff, edk2-devel-01 Cc: Kinney, Michael D, Justen, Jordan L, Paolo Bonzini On 11/22/16 08:20, Fan, Jeff wrote: > Reviewed-by: Jeff Fan <jeff.fan@intel.com> Thank Jeff! I committed this patch in isolation (b43dd22981b7), because I think it has value without the OvmfPkg changes too. Plus, I think the OvmfPkg changes might take longer to get into the tree, and I wouldn't like PiSmmCpuDxeSmm to diverge in the meantime. Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Friday, November 18, 2016 9:53 PM > To: edk2-devel-01 > Cc: Fan, Jeff; Justen, Jordan L; Kinney, Michael D; Paolo Bonzini > Subject: [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode > > Move the declaration of these PCDs from the > > [PcdsFixedAtBuild, PcdsPatchableInModule] > > section of "UefiCpuPkg/UefiCpuPkg.dec" to the > > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > > section. Their types, default values, and token values remain unchanged. > > Only UefiCpuPkg/PiSmmCpuDxeSmm consumes these PCDs, specifically on the call stack of its entry point function, and it turns them into static or dynamically allocated data in SMRAM: > > PiCpuSmmEntry() [PiSmmCpuDxeSmm.c] > InitializeSmmTimer() [SyncTimer.c] > PcdCpuSmmApSyncTimeout > -> mTimeoutTicker > InitializeMpServiceData() [MpService.c] > InitializeMpSyncData() [MpService.c] > PcdCpuSmmSyncMode > -> mSmmMpSyncData->EffectiveSyncMode > > However, there's another call path to fetching "PcdCpuSmmSyncMode", namely > > SmmInitHandler() [PiSmmCpuDxeSmm.c] > InitializeMpSyncData() [MpService.c] > PcdCpuSmmSyncMode > -> mSmmMpSyncData->EffectiveSyncMode > > and this path is exercised during S3 resume (as stated by the comment in > SmmInitHandler() too, "Initialize private data during S3 resume"). > > While we can call the PCD protocol (via PcdLib) for fetching dynamic PCDs in the entry point function, we cannot do that at S3 resume. Therefore pre-fetch PcdCpuSmmSyncMode into a new global variable (which lives in > SMRAM) in InitializeMpServiceData(), just before calling InitializeMpSyncData(). This way InitializeMpSyncData() can retrieve the stashed PCD value from SMRAM, regardless of the boot mode. > > Cc: Jeff Fan <jeff.fan@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v2: > - new in v2 > > UefiCpuPkg/UefiCpuPkg.dec | 20 ++++++++++---------- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 4 +++- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index 89779c447d50..ca560398bbef 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -156,10 +156,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > # @Prompt Processor stack size in SMM. > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x2000|UINT32|0x32132105 > > - ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. > - # @Prompt AP synchronization timeout value in SMM. > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104 > - > ## Indicates if SMM Code Access Check is enabled. > # If enabled, the SMM handler cannot execute the code outside SMM regions. > # This PCD is suggested to TRUE in production image.<BR><BR> @@ -168,12 +164,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > # @Prompt SMM Code Access Check. > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable|TRUE|BOOLEAN|0x60000013 > > - ## Indicates the CPU synchronization method used when processing an SMI. > - # 0x00 - Traditional CPU synchronization method.<BR> > - # 0x01 - Relaxed CPU synchronization method.<BR> > - # @Prompt SMM CPU Synchronization Method. > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 > - > ## Specifies the number of variable MTRRs reserved for OS use. The default number of > # MTRRs reserved for OS use is 2. > # @Prompt Number of reserved variable MTRRs. > @@ -214,6 +204,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > # @Prompt Use static page table for all memory in SMM. > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D > > + ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. > + # @Prompt AP synchronization timeout value in SMM. > + > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x3213 > + 2104 > + > + ## Indicates the CPU synchronization method used when processing an SMI. > + # 0x00 - Traditional CPU synchronization method.<BR> > + # 0x01 - Relaxed CPU synchronization method.<BR> > + # @Prompt SMM CPU Synchronization Method. > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 > + > [PcdsDynamic, PcdsDynamicEx] > ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA. > # @Prompt The pointer to a CPU S3 data buffer. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 9b8db90ff6ed..cfbf59e8f2ca 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -24,6 +24,7 @@ UINTN mSmmMpSyncDataSize; > SMM_CPU_SEMAPHORES mSmmCpuSemaphores; > UINTN mSemaphoreSize; > SPIN_LOCK *mPFLock = NULL; > +SMM_CPU_SYNC_MODE mCpuSmmSyncMode; > > /** > Performs an atomic compare exchange operation to get semaphore. > @@ -1338,7 +1339,7 @@ InitializeMpSyncData ( > // > mSmmMpSyncData->BspIndex = (UINT32)-1; > } > - mSmmMpSyncData->EffectiveSyncMode = (SMM_CPU_SYNC_MODE) PcdGet8 (PcdCpuSmmSyncMode); > + mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode; > > mSmmMpSyncData->Counter = mSmmCpuSemaphores.SemaphoreGlobal.Counter; > mSmmMpSyncData->InsideSmm = mSmmCpuSemaphores.SemaphoreGlobal.InsideSmm; > @@ -1392,6 +1393,7 @@ InitializeMpServiceData ( > (sizeof (SMM_CPU_DATA_BLOCK) + sizeof (BOOLEAN)) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; > mSmmMpSyncData = (SMM_DISPATCHER_MP_SYNC_DATA*) AllocatePages (EFI_SIZE_TO_PAGES (mSmmMpSyncDataSize)); > ASSERT (mSmmMpSyncData != NULL); > + mCpuSmmSyncMode = (SMM_CPU_SYNC_MODE)PcdGet8 (PcdCpuSmmSyncMode); > InitializeMpSyncData (); > > // > -- > 2.9.2 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode 2016-11-22 8:06 ` Laszlo Ersek @ 2016-11-22 8:09 ` Laszlo Ersek 0 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-22 8:09 UTC (permalink / raw) To: Fan, Jeff, edk2-devel-01 Cc: Kinney, Michael D, Justen, Jordan L, Paolo Bonzini On 11/22/16 09:06, Laszlo Ersek wrote: > On 11/22/16 08:20, Fan, Jeff wrote: >> Reviewed-by: Jeff Fan <jeff.fan@intel.com> > > Thank Jeff! This was supposed to be "Thank you, Jeff". Not making much progress on the "getting enough sleep" front. :/ Laszlo > I committed this patch in isolation (b43dd22981b7), because I think it > has value without the OvmfPkg changes too. Plus, I think the OvmfPkg > changes might take longer to get into the tree, and I wouldn't like > PiSmmCpuDxeSmm to diverge in the meantime. > > Thanks! > Laszlo > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Friday, November 18, 2016 9:53 PM >> To: edk2-devel-01 >> Cc: Fan, Jeff; Justen, Jordan L; Kinney, Michael D; Paolo Bonzini >> Subject: [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode >> >> Move the declaration of these PCDs from the >> >> [PcdsFixedAtBuild, PcdsPatchableInModule] >> >> section of "UefiCpuPkg/UefiCpuPkg.dec" to the >> >> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] >> >> section. Their types, default values, and token values remain unchanged. >> >> Only UefiCpuPkg/PiSmmCpuDxeSmm consumes these PCDs, specifically on the call stack of its entry point function, and it turns them into static or dynamically allocated data in SMRAM: >> >> PiCpuSmmEntry() [PiSmmCpuDxeSmm.c] >> InitializeSmmTimer() [SyncTimer.c] >> PcdCpuSmmApSyncTimeout >> -> mTimeoutTicker >> InitializeMpServiceData() [MpService.c] >> InitializeMpSyncData() [MpService.c] >> PcdCpuSmmSyncMode >> -> mSmmMpSyncData->EffectiveSyncMode >> >> However, there's another call path to fetching "PcdCpuSmmSyncMode", namely >> >> SmmInitHandler() [PiSmmCpuDxeSmm.c] >> InitializeMpSyncData() [MpService.c] >> PcdCpuSmmSyncMode >> -> mSmmMpSyncData->EffectiveSyncMode >> >> and this path is exercised during S3 resume (as stated by the comment in >> SmmInitHandler() too, "Initialize private data during S3 resume"). >> >> While we can call the PCD protocol (via PcdLib) for fetching dynamic PCDs in the entry point function, we cannot do that at S3 resume. Therefore pre-fetch PcdCpuSmmSyncMode into a new global variable (which lives in >> SMRAM) in InitializeMpServiceData(), just before calling InitializeMpSyncData(). This way InitializeMpSyncData() can retrieve the stashed PCD value from SMRAM, regardless of the boot mode. >> >> Cc: Jeff Fan <jeff.fan@intel.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Michael Kinney <michael.d.kinney@intel.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> v2: >> - new in v2 >> >> UefiCpuPkg/UefiCpuPkg.dec | 20 ++++++++++---------- >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 4 +++- >> 2 files changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index 89779c447d50..ca560398bbef 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.dec >> +++ b/UefiCpuPkg/UefiCpuPkg.dec >> @@ -156,10 +156,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] >> # @Prompt Processor stack size in SMM. >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x2000|UINT32|0x32132105 >> >> - ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. >> - # @Prompt AP synchronization timeout value in SMM. >> - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104 >> - >> ## Indicates if SMM Code Access Check is enabled. >> # If enabled, the SMM handler cannot execute the code outside SMM regions. >> # This PCD is suggested to TRUE in production image.<BR><BR> @@ -168,12 +164,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] >> # @Prompt SMM Code Access Check. >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable|TRUE|BOOLEAN|0x60000013 >> >> - ## Indicates the CPU synchronization method used when processing an SMI. >> - # 0x00 - Traditional CPU synchronization method.<BR> >> - # 0x01 - Relaxed CPU synchronization method.<BR> >> - # @Prompt SMM CPU Synchronization Method. >> - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 >> - >> ## Specifies the number of variable MTRRs reserved for OS use. The default number of >> # MTRRs reserved for OS use is 2. >> # @Prompt Number of reserved variable MTRRs. >> @@ -214,6 +204,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] >> # @Prompt Use static page table for all memory in SMM. >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D >> >> + ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM. >> + # @Prompt AP synchronization timeout value in SMM. >> + >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x3213 >> + 2104 >> + >> + ## Indicates the CPU synchronization method used when processing an SMI. >> + # 0x00 - Traditional CPU synchronization method.<BR> >> + # 0x01 - Relaxed CPU synchronization method.<BR> >> + # @Prompt SMM CPU Synchronization Method. >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014 >> + >> [PcdsDynamic, PcdsDynamicEx] >> ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA. >> # @Prompt The pointer to a CPU S3 data buffer. >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> index 9b8db90ff6ed..cfbf59e8f2ca 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> @@ -24,6 +24,7 @@ UINTN mSmmMpSyncDataSize; >> SMM_CPU_SEMAPHORES mSmmCpuSemaphores; >> UINTN mSemaphoreSize; >> SPIN_LOCK *mPFLock = NULL; >> +SMM_CPU_SYNC_MODE mCpuSmmSyncMode; >> >> /** >> Performs an atomic compare exchange operation to get semaphore. >> @@ -1338,7 +1339,7 @@ InitializeMpSyncData ( >> // >> mSmmMpSyncData->BspIndex = (UINT32)-1; >> } >> - mSmmMpSyncData->EffectiveSyncMode = (SMM_CPU_SYNC_MODE) PcdGet8 (PcdCpuSmmSyncMode); >> + mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode; >> >> mSmmMpSyncData->Counter = mSmmCpuSemaphores.SemaphoreGlobal.Counter; >> mSmmMpSyncData->InsideSmm = mSmmCpuSemaphores.SemaphoreGlobal.InsideSmm; >> @@ -1392,6 +1393,7 @@ InitializeMpServiceData ( >> (sizeof (SMM_CPU_DATA_BLOCK) + sizeof (BOOLEAN)) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; >> mSmmMpSyncData = (SMM_DISPATCHER_MP_SYNC_DATA*) AllocatePages (EFI_SIZE_TO_PAGES (mSmmMpSyncDataSize)); >> ASSERT (mSmmMpSyncData != NULL); >> + mCpuSmmSyncMode = (SMM_CPU_SYNC_MODE)PcdGet8 (PcdCpuSmmSyncMode); >> InitializeMpSyncData (); >> >> // >> -- >> 2.9.2 >> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] OvmfPkg: dynamic defaults for PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode 2016-11-18 13:52 [PATCH v2 0/4] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek @ 2016-11-18 13:52 ` Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 3/4] OvmfPkg/IndustryStandard: add macros for QEMU's SMI feature control bits Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek 3 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-18 13:52 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan, Jordan Justen, Michael Kinney, Paolo Bonzini Move the platform-specific default values for these PCDs from the [PcdsFixedAtBuild] / [PcdsFixedAtBuild.X64] sections to the [PcdsDynamicDefault] section. Cc: Jeff Fan <jeff.fan@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new in v2 OvmfPkg/OvmfPkgIa32.dsc | 7 +++++-- OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++-- OvmfPkg/OvmfPkgX64.dsc | 7 +++++-- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index ed43c4514491..239751741a69 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -438,8 +438,6 @@ [PcdsFixedAtBuild] !endif !if $(SMM_REQUIRE) == TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 !endif @@ -488,6 +486,11 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE +!if $(SMM_REQUIRE) == TRUE + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 +!endif + ################################################################################ # # Components Section - list of all EDK II Modules needed by this Platform. diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index ccd156d9231a..ba8dd2baf917 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -444,8 +444,6 @@ [PcdsFixedAtBuild.X64] !endif !if $(SMM_REQUIRE) == TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 !endif @@ -496,6 +494,11 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE +!if $(SMM_REQUIRE) == TRUE + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 +!endif + ################################################################################ # # Components Section - list of all EDK II Modules needed by this Platform. diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 012ce85462c5..ba905329f877 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -443,8 +443,6 @@ [PcdsFixedAtBuild] !endif !if $(SMM_REQUIRE) == TRUE - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 !endif @@ -495,6 +493,11 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE +!if $(SMM_REQUIRE) == TRUE + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 +!endif + ################################################################################ # # Components Section - list of all EDK II Modules needed by this Platform. -- 2.9.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] OvmfPkg/IndustryStandard: add macros for QEMU's SMI feature control bits 2016-11-18 13:52 [PATCH v2 0/4] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 2/4] OvmfPkg: dynamic defaults for " Laszlo Ersek @ 2016-11-18 13:52 ` Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek 3 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-11-18 13:52 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan, Jordan Justen, Michael Kinney, Paolo Bonzini The bits are specified in "docs/specs/q35-apm-sts.txt" in the QEMU tree. Cc: Jeff Fan <jeff.fan@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - new in v2 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h index 4dc2c39901c1..5848c5202912 100644 --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h @@ -91,8 +91,13 @@ // // IO ports // -#define ICH9_APM_CNT 0xB2 -#define ICH9_APM_STS 0xB3 +#define ICH9_APM_CNT 0xB2 +#define ICH9_APM_STS 0xB3 +// +// The following bits are QEMU extensions. +// +#define QEMU_ICH9_APM_STS_GET_SET_FEAT BIT1 +#define QEMU_ICH9_APM_STS_F_BCAST_SMI BIT2 // // IO ports relative to PMBASE -- 2.9.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available 2016-11-18 13:52 [PATCH v2 0/4] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek ` (2 preceding siblings ...) 2016-11-18 13:52 ` [PATCH v2 3/4] OvmfPkg/IndustryStandard: add macros for QEMU's SMI feature control bits Laszlo Ersek @ 2016-11-18 13:52 ` Laszlo Ersek 2016-11-23 3:44 ` Jordan Justen 3 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2016-11-18 13:52 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan, Jordan Justen, Michael Kinney, Paolo Bonzini When writing to IO port 0xB2 (ICH9_APM_CNT), QEMU by default injects an SMI only on the VCPU that is writing the port. This has exposed corner cases and strange behavior with edk2 code, which generally expects a software SMI to affect all CPUs at once. We've experienced instability despite the fact that OVMF sets PcdCpuSmmApSyncTimeout and PcdCpuSmmSyncMode differently from the UefiCpuPkg defaults, such that they match QEMU's unicast SMIs better. (Refer to edk2 commits 9b1e378811ff and bb0f18b0bce6.) Using the feature negotiation specified in QEMU's "docs/specs/q35-apm-sts.txt", we can ask QEMU to broadcast SMIs. Extensive testing from earlier proves that broadcast SMIs are only reliable if we use the UefiCpuPkg defaults for the above PCDs. With those settings however, the broadcast is very reliable -- the most reliable configuration encountered thus far. Therefore negotiate broadcast SMIs with QEMU, and if the negotiation is successful, dynamically revert the PCDs to the UefiCpuPkg defaults. Setting the PCDs in this module is safe: - only PiSmmCpuDxeSmm consumes them, - PiSmmCpuDxeSmm is a DXE_SMM_DRIVER, launched by the SMM_CORE (MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf), - the SMM_CORE is launched by the SMM IPL runtime DXE driver (MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf), - the SMM IPL has a DEPEX on EFI_SMM_CONTROL2_PROTOCOL, - OvmfPkg/SmmControl2Dxe produces that protocol. The end result is that PiSmmCpuDxeSmm cannot be dispatched before SmmControl2Dxe installs EFI_SMM_CONTROL2_PROTOCOL and returns from its entry point. Hence we can set the PCD's consumed by PiSmmCpuDxeSmm in SmmControl2Dxe. Cc: Jeff Fan <jeff.fan@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v2: - negotiate the broadcast SMI feature - make the S3 boot script re-select it if it's available at first boot - set PiSmmCpuDxeSmm's PCD's dynamically OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 5 ++ OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 73 +++++++++++++++++++- 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf index 0e9f98c2871c..c28832435eed 100644 --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf @@ -44,6 +44,7 @@ [Sources] [Packages] MdePkg/MdePkg.dec OvmfPkg/OvmfPkg.dec + UefiCpuPkg/UefiCpuPkg.dec [LibraryClasses] BaseLib @@ -59,6 +60,10 @@ [Protocols] gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES gEfiSmmControl2ProtocolGuid ## PRODUCES +[Pcd] + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES + [FeaturePcd] gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c index 82549b0a7e35..6f05797979de 100644 --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c @@ -56,6 +56,15 @@ OnS3SaveStateInstalled ( STATIC UINTN mSmiEnable; // +// The indicator whether we have negotiated with QEMU to broadcast the SMI to +// all VCPUs whenever we write to ICH9_APM_CNT in our +// EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. This variable is only +// used to carry information from the entry point function to the S3SaveState +// protocol installation callback. +// +STATIC BOOLEAN mSmiBroadcast; + +// // Event signaled when an S3SaveState protocol interface is installed. // STATIC EFI_EVENT mS3SaveStateInstalled; @@ -107,10 +116,10 @@ SmmControl2DxeTrigger ( // 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. + // QEMU utilizes the status register for feature negotiation, therefore we + // can't accept external data. // - IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); + ASSERT (DataPort == NULL); IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort); return EFI_SUCCESS; } @@ -177,6 +186,7 @@ SmmControl2DxeEntryPoint ( { UINT32 PmBase; UINT32 SmiEnableVal; + UINT8 ApmStatusVal; EFI_STATUS Status; // @@ -229,6 +239,43 @@ SmmControl2DxeEntryPoint ( goto FatalError; } + // + // Negotiate broadcast SMI with QEMU. + // + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_GET_SET_FEAT); + ApmStatusVal = IoRead8 (ICH9_APM_STS); + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { + DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation unavailable\n", + __FUNCTION__)); + IoWrite8 (ICH9_APM_STS, 0); + } else if ((ApmStatusVal & QEMU_ICH9_APM_STS_F_BCAST_SMI) == 0) { + DEBUG ((DEBUG_VERBOSE, "%a: SMI broadcast unavailable\n", __FUNCTION__)); + } else { + // + // Request the broadcast feature, and nothing else. Check for confirmation. + // + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_F_BCAST_SMI); + ApmStatusVal = IoRead8 (ICH9_APM_STS); + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { + DEBUG ((DEBUG_VERBOSE, "%a: failed to negotiate SMI broadcast\n", + __FUNCTION__)); + } else { + // + // Configure the traditional AP sync / SMI delivery mode for + // PiSmmCpuDxeSmm. Effectively, restore the UefiCpuPkg defaults, from + // which the original QEMU behavior (i.e., unicast SMI) used to differ. + // + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration failed\n", + __FUNCTION__)); + goto FatalError; + } + mSmiBroadcast = TRUE; + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__)); + } + } + if (QemuFwCfgS3Enabled ()) { VOID *Registration; @@ -360,6 +407,26 @@ OnS3SaveStateInstalled ( CpuDeadLoop (); } + if (mSmiBroadcast) { + UINT8 ApmStatusVal; + + ApmStatusVal = QEMU_ICH9_APM_STS_F_BCAST_SMI; + Status = S3SaveState->Write ( + S3SaveState, + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, + EfiBootScriptWidthUint8, + (UINT64)ICH9_APM_STS, + (UINTN)1, + &ApmStatusVal + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", + __FUNCTION__, Status)); + ASSERT (FALSE); + CpuDeadLoop (); + } + } + DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__)); gBS->CloseEvent (Event); mS3SaveStateInstalled = NULL; -- 2.9.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available 2016-11-18 13:52 ` [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek @ 2016-11-23 3:44 ` Jordan Justen 2016-11-23 15:44 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Jordan Justen @ 2016-11-23 3:44 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Michael Kinney, Jeff Fan, Paolo Bonzini On 2016-11-18 05:52:49, Laszlo Ersek wrote: > When writing to IO port 0xB2 (ICH9_APM_CNT), QEMU by default injects an > SMI only on the VCPU that is writing the port. This has exposed corner > cases and strange behavior with edk2 code, which generally expects a > software SMI to affect all CPUs at once. We've experienced instability > despite the fact that OVMF sets PcdCpuSmmApSyncTimeout and > PcdCpuSmmSyncMode differently from the UefiCpuPkg defaults, such that they > match QEMU's unicast SMIs better. (Refer to edk2 commits 9b1e378811ff and > bb0f18b0bce6.) > > Using the feature negotiation specified in QEMU's > "docs/specs/q35-apm-sts.txt", we can ask QEMU to broadcast SMIs. > Extensive testing from earlier proves that broadcast SMIs are only > reliable if we use the UefiCpuPkg defaults for the above PCDs. With those > settings however, the broadcast is very reliable -- the most reliable > configuration encountered thus far. > > Therefore negotiate broadcast SMIs with QEMU, and if the negotiation is > successful, dynamically revert the PCDs to the UefiCpuPkg defaults. > > Setting the PCDs in this module is safe: > > - only PiSmmCpuDxeSmm consumes them, > > - PiSmmCpuDxeSmm is a DXE_SMM_DRIVER, launched by the SMM_CORE > (MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf), > > - the SMM_CORE is launched by the SMM IPL runtime DXE driver > (MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf), > > - the SMM IPL has a DEPEX on EFI_SMM_CONTROL2_PROTOCOL, > > - OvmfPkg/SmmControl2Dxe produces that protocol. > > The end result is that PiSmmCpuDxeSmm cannot be dispatched before > SmmControl2Dxe installs EFI_SMM_CONTROL2_PROTOCOL and returns from its > entry point. Hence we can set the PCD's consumed by PiSmmCpuDxeSmm in > SmmControl2Dxe. > > Cc: Jeff Fan <jeff.fan@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v2: > - negotiate the broadcast SMI feature > - make the S3 boot script re-select it if it's available at first boot > - set PiSmmCpuDxeSmm's PCD's dynamically > > OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 5 ++ > OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 73 +++++++++++++++++++- > 2 files changed, 75 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > index 0e9f98c2871c..c28832435eed 100644 > --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > @@ -44,6 +44,7 @@ [Sources] > [Packages] > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > > [LibraryClasses] > BaseLib > @@ -59,6 +60,10 @@ [Protocols] > gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES > gEfiSmmControl2ProtocolGuid ## PRODUCES > > +[Pcd] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES > + > [FeaturePcd] > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > index 82549b0a7e35..6f05797979de 100644 > --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > @@ -56,6 +56,15 @@ OnS3SaveStateInstalled ( > STATIC UINTN mSmiEnable; > // > +// The indicator whether we have negotiated with QEMU to broadcast the SMI to > +// all VCPUs whenever we write to ICH9_APM_CNT in our > +// EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. This variable is only > +// used to carry information from the entry point function to the S3SaveState > +// protocol installation callback. > +// > +STATIC BOOLEAN mSmiBroadcast; I think you are relying on uninitialized static data being zero'd. Does it hurt to actually set it to FALSE? I'm glad we'll be using a mechanism that broadcasts to all the processors like the real hardware. It is a bit unfortunate that it doesn't go through the b2 port for it. Did you get a chance to test variable writes on Windows with this change? I know Windows can be more picky about port accesses... Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> Thanks! > + > +// > // Event signaled when an S3SaveState protocol interface is installed. > // > STATIC EFI_EVENT mS3SaveStateInstalled; > @@ -107,10 +116,10 @@ SmmControl2DxeTrigger ( > // 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. > + // QEMU utilizes the status register for feature negotiation, therefore we > + // can't accept external data. > // > - IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); > + ASSERT (DataPort == NULL); > IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort); > return EFI_SUCCESS; > } > @@ -177,6 +186,7 @@ SmmControl2DxeEntryPoint ( > { > UINT32 PmBase; > UINT32 SmiEnableVal; > + UINT8 ApmStatusVal; > EFI_STATUS Status; > > // > @@ -229,6 +239,43 @@ SmmControl2DxeEntryPoint ( > goto FatalError; > } > > + // > + // Negotiate broadcast SMI with QEMU. > + // > + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_GET_SET_FEAT); > + ApmStatusVal = IoRead8 (ICH9_APM_STS); > + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { > + DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation unavailable\n", > + __FUNCTION__)); > + IoWrite8 (ICH9_APM_STS, 0); > + } else if ((ApmStatusVal & QEMU_ICH9_APM_STS_F_BCAST_SMI) == 0) { > + DEBUG ((DEBUG_VERBOSE, "%a: SMI broadcast unavailable\n", __FUNCTION__)); > + } else { > + // > + // Request the broadcast feature, and nothing else. Check for confirmation. > + // > + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_F_BCAST_SMI); > + ApmStatusVal = IoRead8 (ICH9_APM_STS); > + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { > + DEBUG ((DEBUG_VERBOSE, "%a: failed to negotiate SMI broadcast\n", > + __FUNCTION__)); > + } else { > + // > + // Configure the traditional AP sync / SMI delivery mode for > + // PiSmmCpuDxeSmm. Effectively, restore the UefiCpuPkg defaults, from > + // which the original QEMU behavior (i.e., unicast SMI) used to differ. > + // > + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || > + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { > + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration failed\n", > + __FUNCTION__)); > + goto FatalError; > + } > + mSmiBroadcast = TRUE; > + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__)); > + } > + } > + > if (QemuFwCfgS3Enabled ()) { > VOID *Registration; > > @@ -360,6 +407,26 @@ OnS3SaveStateInstalled ( > CpuDeadLoop (); > } > > + if (mSmiBroadcast) { > + UINT8 ApmStatusVal; > + > + ApmStatusVal = QEMU_ICH9_APM_STS_F_BCAST_SMI; > + Status = S3SaveState->Write ( > + S3SaveState, > + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, > + EfiBootScriptWidthUint8, > + (UINT64)ICH9_APM_STS, > + (UINTN)1, > + &ApmStatusVal > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", > + __FUNCTION__, Status)); > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + } > + > DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__)); > gBS->CloseEvent (Event); > mS3SaveStateInstalled = NULL; > -- > 2.9.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available 2016-11-23 3:44 ` Jordan Justen @ 2016-11-23 15:44 ` Laszlo Ersek 2016-12-01 20:30 ` Laszlo Ersek 0 siblings, 1 reply; 11+ messages in thread From: Laszlo Ersek @ 2016-11-23 15:44 UTC (permalink / raw) To: Jordan Justen, edk2-devel-01; +Cc: Michael Kinney, Jeff Fan, Paolo Bonzini On 11/23/16 04:44, Jordan Justen wrote: > On 2016-11-18 05:52:49, Laszlo Ersek wrote: >> When writing to IO port 0xB2 (ICH9_APM_CNT), QEMU by default injects an >> SMI only on the VCPU that is writing the port. This has exposed corner >> cases and strange behavior with edk2 code, which generally expects a >> software SMI to affect all CPUs at once. We've experienced instability >> despite the fact that OVMF sets PcdCpuSmmApSyncTimeout and >> PcdCpuSmmSyncMode differently from the UefiCpuPkg defaults, such that they >> match QEMU's unicast SMIs better. (Refer to edk2 commits 9b1e378811ff and >> bb0f18b0bce6.) >> >> Using the feature negotiation specified in QEMU's >> "docs/specs/q35-apm-sts.txt", we can ask QEMU to broadcast SMIs. >> Extensive testing from earlier proves that broadcast SMIs are only >> reliable if we use the UefiCpuPkg defaults for the above PCDs. With those >> settings however, the broadcast is very reliable -- the most reliable >> configuration encountered thus far. >> >> Therefore negotiate broadcast SMIs with QEMU, and if the negotiation is >> successful, dynamically revert the PCDs to the UefiCpuPkg defaults. >> >> Setting the PCDs in this module is safe: >> >> - only PiSmmCpuDxeSmm consumes them, >> >> - PiSmmCpuDxeSmm is a DXE_SMM_DRIVER, launched by the SMM_CORE >> (MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf), >> >> - the SMM_CORE is launched by the SMM IPL runtime DXE driver >> (MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf), >> >> - the SMM IPL has a DEPEX on EFI_SMM_CONTROL2_PROTOCOL, >> >> - OvmfPkg/SmmControl2Dxe produces that protocol. >> >> The end result is that PiSmmCpuDxeSmm cannot be dispatched before >> SmmControl2Dxe installs EFI_SMM_CONTROL2_PROTOCOL and returns from its >> entry point. Hence we can set the PCD's consumed by PiSmmCpuDxeSmm in >> SmmControl2Dxe. >> >> Cc: Jeff Fan <jeff.fan@intel.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Michael Kinney <michael.d.kinney@intel.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> v2: >> - negotiate the broadcast SMI feature >> - make the S3 boot script re-select it if it's available at first boot >> - set PiSmmCpuDxeSmm's PCD's dynamically >> >> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 5 ++ >> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 73 +++++++++++++++++++- >> 2 files changed, 75 insertions(+), 3 deletions(-) >> >> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf >> index 0e9f98c2871c..c28832435eed 100644 >> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf >> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf >> @@ -44,6 +44,7 @@ [Sources] >> [Packages] >> MdePkg/MdePkg.dec >> OvmfPkg/OvmfPkg.dec >> + UefiCpuPkg/UefiCpuPkg.dec >> >> [LibraryClasses] >> BaseLib >> @@ -59,6 +60,10 @@ [Protocols] >> gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES >> gEfiSmmControl2ProtocolGuid ## PRODUCES >> >> +[Pcd] >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES >> + >> [FeaturePcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire >> >> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> index 82549b0a7e35..6f05797979de 100644 >> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c >> @@ -56,6 +56,15 @@ OnS3SaveStateInstalled ( >> STATIC UINTN mSmiEnable; >> // >> +// The indicator whether we have negotiated with QEMU to broadcast the SMI to >> +// all VCPUs whenever we write to ICH9_APM_CNT in our >> +// EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. This variable is only >> +// used to carry information from the entry point function to the S3SaveState >> +// protocol installation callback. >> +// >> +STATIC BOOLEAN mSmiBroadcast; > > I think you are relying on uninitialized static data being zero'd. Auto-initializing mSmiBroadcast to FALSE (0) here is required by the C standard. I think you are recalling the obscure problem we have with PEIMs on S3 resume, in the SMM-less build only. (And, I'm impressed that you remember it :)) In that case we re-use the previously decompressed (and executed) PEI modules from RAM. Then such variables (= of static storage duration) are not re-initialized when they are dispatched during S3 resume (in the SMM-less build). However, as far as I remember, they are also not re-set when we provide an initializer of the form "= FALSE" -- that's again not code in the binary, just initialized static data. The re-setting only happens (at SMM-less S3 resume) if we assign values in code (entry point function of the module, or library constructor). Anyhow, this module is a runtime DXE driver, not a PEIM, so it is not affected. During S3 resume, its not (re)dispatched, it plainly survives in runtime code / data type memory, and all its variables are supposed to retain their values. > > Does it hurt to actually set it to FALSE? It does not hurt, but it makes no difference; either in this case, or for those PEIMs during SMM-less S3 resume. > > I'm glad we'll be using a mechanism that broadcasts to all the > processors like the real hardware. It is a bit unfortunate that it > doesn't go through the b2 port for it. > > Did you get a chance to test variable writes on Windows with this > change? I know Windows can be more picky about port accesses... Before posting the QEMU and edk2 series, I tested Windows extensively with broadcast SMI (using the Ia32X64 build): { Windows 7, Windows Server 2008 R2, Windows 8.1, Windows Server 2012 R2, Windows 10, Windows Server 2016 Tech Preview 4 } x { normal boot, S3 resume } In particular, the Windows 8 family boots *much* faster with broadcast SMIs. I had KVM-traced Windows 8 boot earlier, and for some reason that boot is crazy about messing with variables -- the KVM trace was chock-full of SMIs. A good part of those variable accesses seemed to come from APs, which made a visible impact (--> choppiness) on the rotating animation during Windows 8 boot. So the broadcast SMI happens to fix that mild annoyance as well. Anyway, now that you are asking, I remember another method to trigger a variable write from under Windows 8: https://firmware.intel.com/blog/using-os-indications-uefi http://answers.microsoft.com/en-us/surface/forum/surfpro-surfgetstart/accessing-windows-8-firmware-settings/c37b0b00-ae8f-4ad8-9ad6-88e070cb4d36 http://www.windowspasswordsrecovery.com/articles/win8/how-to-access-the-boot-menu-and-bios-in-a-windows-8-computer.html Windows 8 provides a GUI to set the OsIndications UEFI variable. So, I've just retested it now (with SMI broadcast), and it works fine. Windows reboots the VM and the setup TUI is entered immediately. From the log: ----------- SetBootOrderFromQemu: setting BootOrder: success [Bds]OsIndication: 0000000000000001 [Bds]=============Begin Load Options Dumping ...============= Driver Options: SysPrep Options: Boot Options: Boot0002: UEFI QEMU QEMU CD-ROM 2 0x0001 Boot0003: Windows Boot Manager 0x0001 Boot0001: UEFI QEMU QEMU CD-ROM 0x0001 Boot0000: UiApp 0x0109 Boot0009: EFI Internal Shell 0x0001 PlatformRecovery Options: PlatformRecovery0000: Default PlatformRecovery 0x0001 [Bds]=============End Load Options Dumping============= [Bds] Booting Boot Manager Menu. ----------- > > Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> Thank you! I'll delay pushing the patches until Michael Tsirkin reviews the interface spec in the QEMU series on qemu-devel. Thanks! Laszlo > Thanks! > >> + >> +// >> // Event signaled when an S3SaveState protocol interface is installed. >> // >> STATIC EFI_EVENT mS3SaveStateInstalled; >> @@ -107,10 +116,10 @@ SmmControl2DxeTrigger ( >> // 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. >> + // QEMU utilizes the status register for feature negotiation, therefore we >> + // can't accept external data. >> // >> - IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); >> + ASSERT (DataPort == NULL); >> IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort); >> return EFI_SUCCESS; >> } >> @@ -177,6 +186,7 @@ SmmControl2DxeEntryPoint ( >> { >> UINT32 PmBase; >> UINT32 SmiEnableVal; >> + UINT8 ApmStatusVal; >> EFI_STATUS Status; >> >> // >> @@ -229,6 +239,43 @@ SmmControl2DxeEntryPoint ( >> goto FatalError; >> } >> >> + // >> + // Negotiate broadcast SMI with QEMU. >> + // >> + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_GET_SET_FEAT); >> + ApmStatusVal = IoRead8 (ICH9_APM_STS); >> + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { >> + DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation unavailable\n", >> + __FUNCTION__)); >> + IoWrite8 (ICH9_APM_STS, 0); >> + } else if ((ApmStatusVal & QEMU_ICH9_APM_STS_F_BCAST_SMI) == 0) { >> + DEBUG ((DEBUG_VERBOSE, "%a: SMI broadcast unavailable\n", __FUNCTION__)); >> + } else { >> + // >> + // Request the broadcast feature, and nothing else. Check for confirmation. >> + // >> + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_F_BCAST_SMI); >> + ApmStatusVal = IoRead8 (ICH9_APM_STS); >> + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { >> + DEBUG ((DEBUG_VERBOSE, "%a: failed to negotiate SMI broadcast\n", >> + __FUNCTION__)); >> + } else { >> + // >> + // Configure the traditional AP sync / SMI delivery mode for >> + // PiSmmCpuDxeSmm. Effectively, restore the UefiCpuPkg defaults, from >> + // which the original QEMU behavior (i.e., unicast SMI) used to differ. >> + // >> + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || >> + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { >> + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration failed\n", >> + __FUNCTION__)); >> + goto FatalError; >> + } >> + mSmiBroadcast = TRUE; >> + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__)); >> + } >> + } >> + >> if (QemuFwCfgS3Enabled ()) { >> VOID *Registration; >> >> @@ -360,6 +407,26 @@ OnS3SaveStateInstalled ( >> CpuDeadLoop (); >> } >> >> + if (mSmiBroadcast) { >> + UINT8 ApmStatusVal; >> + >> + ApmStatusVal = QEMU_ICH9_APM_STS_F_BCAST_SMI; >> + Status = S3SaveState->Write ( >> + S3SaveState, >> + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, >> + EfiBootScriptWidthUint8, >> + (UINT64)ICH9_APM_STS, >> + (UINTN)1, >> + &ApmStatusVal >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", >> + __FUNCTION__, Status)); >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> + } >> + } >> + >> DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__)); >> gBS->CloseEvent (Event); >> mS3SaveStateInstalled = NULL; >> -- >> 2.9.2 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available 2016-11-23 15:44 ` Laszlo Ersek @ 2016-12-01 20:30 ` Laszlo Ersek 0 siblings, 0 replies; 11+ messages in thread From: Laszlo Ersek @ 2016-12-01 20:30 UTC (permalink / raw) To: Jordan Justen; +Cc: edk2-devel-01, Michael Kinney, Paolo Bonzini, Jeff Fan On 11/23/16 16:44, Laszlo Ersek wrote: > On 11/23/16 04:44, Jordan Justen wrote: >> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > > Thank you! > > I'll delay pushing the patches until Michael Tsirkin reviews the > interface spec in the QEMU series on qemu-devel. This version of the series can be dropped. The underlying QEMU patches weren't accepted, and for QEMU I've posted another patch set today: http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00129.html For this I have the corresponding OVMF patches ready (of course), but until the QEMU work converges, there's little point in posting them. I did post some of the prerequisite edk2 / OVMF patches today (which are useful independently, and can be reviewed independently). Thanks, and apologies for the review-in-vain. :/ Laszlo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-12-01 20:30 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-18 13:52 [PATCH v2 0/4] OvmfPkg: broadcast SMIs and dynamically revert to traditional AP sync mode Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 1/4] UefiCpuPkg/PiSmmCpuDxeSmm: dynamic PcdCpuSmmApSyncTimeout, PcdCpuSmmSyncMode Laszlo Ersek 2016-11-22 7:20 ` Fan, Jeff 2016-11-22 8:06 ` Laszlo Ersek 2016-11-22 8:09 ` Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 2/4] OvmfPkg: dynamic defaults for " Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 3/4] OvmfPkg/IndustryStandard: add macros for QEMU's SMI feature control bits Laszlo Ersek 2016-11-18 13:52 ` [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available Laszlo Ersek 2016-11-23 3:44 ` Jordan Justen 2016-11-23 15:44 ` Laszlo Ersek 2016-12-01 20:30 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox