Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval

["Laszlo Ersek" <lersek@redhat.com>]

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|0x0000001E

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.

Thanks,
Laszlo