Laszlo, Yes. We hit the issue at normal boot on some platform.
I agree we could update comments to mention the CandidateBsp array of BOOLEANs also. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, July 19, 2016 7:25 PM To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-de...@ml01.01.org; Fan, Jeff <jeff....@intel.com> Cc: Tian, Feng <feng.t...@intel.com> Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: SMM_CPU_DATA_BLOCK is not cleared On 07/19/16 04:58, Michael Kinney wrote: > From: Jeff Fan <jeff....@intel.com> > > The commit 8b9311 changed the zeroing of mSmmMpSyncData of type > SMM_DISPATCHER_MP_SYNC_DATA by the following patch. > - ZeroMem (mSmmMpSyncData, mSmmMpSyncDataSize); > + mSmmMpSyncData->SwitchBsp = FALSE; > > mSmmMpSyncDataSize not only includes SMM_DISPATCHER_MP_SYNC_DATA, but > also includes the SMM_CPU_DATA_BLOCK array and one BOOLEAN variable > array as shown here: > > mSmmMpSyncDataSize = sizeof (SMM_DISPATCHER_MP_SYNC_DATA) + > (sizeof (SMM_CPU_DATA_BLOCK) + sizeof (BOOLEAN)) * > gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; > > This patch restores the original ZeroMem() to clear all CPU Sync data. > The commit 8b9311 may cause unexpected behavior. > > Cc: Feng Tian <feng.t...@intel.com> > Cc: Michael Kinney <michael.d.kin...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff....@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 5ba0514..fe9076b 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1276,7 +1276,10 @@ InitializeMpSyncData ( > UINTN CpuIndex; > > if (mSmmMpSyncData != NULL) { > - mSmmMpSyncData->SwitchBsp = FALSE; > + // > + // mSmmMpSyncDataSize includes SMM_DISPATCHER_MP_SYNC_DATA and > SMM_CPU_DATA_BLOCK range > + // > + ZeroMem (mSmmMpSyncData, mSmmMpSyncDataSize); > mSmmMpSyncData->CpuData = (SMM_CPU_DATA_BLOCK *)((UINT8 *)mSmmMpSyncData > + sizeof (SMM_DISPATCHER_MP_SYNC_DATA)); > mSmmMpSyncData->CandidateBsp = (BOOLEAN *)(mSmmMpSyncData->CpuData + > gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > if (FeaturePcdGet (PcdCpuSmmEnableBspElection)) { > * Am I right to think that this issue can hit on the normal boot path as well, if the pages allocated by InitializeSmmCpuSemaphores() for this area were allocated and released earlier, by another part of the firmware? Given that this area is allocated from SMRAM, the above should be pretty unlikely though. Are you experiencing the issue at S3 resume only, or even on normal boot? Are there any special circumstances that trigger it? I'm trying to gauge if this is a severe bug that should be backported to our downstream OVMF package "urgently", or if it's okay to get the patch "eventually" (when we next rebase). We haven't seen the issue in practice. * Second, this patch makes me remember 229fd9e7aa1c MdeModulePkg: PiSmmCore: Remove confusing CopyMem() of _ENTRY_CONTEXT Namely, I wonder if it would be clearer to zero out mSmmMpSyncData, mSmmMpSyncData->CpuData, and mSmmMpSyncData->CandidateBsp separately. OTOH, if a new array of some sort was appended, then that would need separate zeroing again, which is easy to forget -- mSmmMpSyncDataSize is pretty clear and robust against further field additions. So I think my only suggestion is to change the comment: // // mSmmMpSyncDataSize includes SMM_DISPATCHER_MP_SYNC_DATA and // other data packed after it // This is motivated by two facts: - the comment is already inaccurate, because it doesn't mention the CandidateBsp array of BOOLEANs - should another field be appended later on, the comment would remain valid The point is that we should warn the reader that mSmmMpSyncDataSize bytes have to be cleared, because there are "other data" after what's apparent. What do you think? Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
