> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, March 25, 2020 9:37 AM > To: Kinney, Michael D; Ni, Ray; Zeng, Star; Wu, Hao A; devel@edk2.groups.io > Cc: Dong, Eric; Brian J . Johnson > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to > control AP status check interval > > On 03/24/20 16:59, Kinney, Michael D wrote: > > How was the milliseconds units selected? > > I suggested msecs for continuity with the pre-patch unit: > > #define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100)) > > > We have other PCDs that provide timer intervals in > > 100ns unit and my microseconds. I would prefer we > > be consistent so platform developers do not have > > to keep track of so many different units for time > > intervals. > > Sounds OK to me. > > What's more: your suggestion points at a bug in the v2 patch. The patch > replaces > the argument for the gBS->SetTimer() service's "TriggerTime" parameter, > namely: > > Status = gBS->SetTimer ( > mCheckAllApsEvent, > TimerPeriodic, > - AP_CHECK_INTERVAL > + PcdGet64 (PcdCpuApStatusCheckInterval) > ); > > But that parameter is taken in units of 100ns, per spec. > > Pre-patch, AP_CHECK_INTERVAL takes that into account: > > #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) > MultU64x32((UINT64)(Milliseconds), 10000) > > but post-patch, the PCD does not: > > + > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x0 > 000001E > > So this patch would shorten the interval by a divisor of 10000 at once, namely > from (100 * 10000) * 100ns, to 100 * 100ns. > > The fix is to update the PCD comments according to your remark, and to change > the default PCD value to (100 * 10000) = 1000,000.
Hello Laszlo, Thanks for the catch. I will address the bug introduced in the V2 patch. One thing to confirm, as Ray mentioned that the UefiCpuPkg uses microseconds(us) as unit for timeout/interval values, so how about using the below definition for the proposed PCD: gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroseconds|100000|UINT32|0x0000001E And in the C code use the macro 'EFI_TIMER_PERIOD_MICROSECONDS' to convert the PCD value to the 'SetTimer' parameter? Best Regards, Hao Wu > > Thanks, > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56233): https://edk2.groups.io/g/devel/message/56233 Mute This Topic: https://groups.io/mt/72512073/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
