Re: [edk2-devel] [PATCH v2 6/7] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Laszlo Ersek <lersek@xxxxxxxxxx>]

On 03/25/21 16:47, Anthony PERARD via groups.io wrote:
> Calculate the frequency of the APIC timer that Xen provides.
> 
> Even though the frequency is currently hard-coded, it isn't part of
> the public ABI that Xen provides and thus may change at any time. OVMF
> needs to determine the frequency by an other mean.
> 
> Fortunately, Xen provides a way to determines the frequency of the
> TSC, so we can use TSC to calibrate the frequency of the APIC timer.
> That information is found in the shared_info page which we map and
> unmap once done (XenBusDxe is going to map the page somewhere else).
> 
> The shared_info page is map at the highest physical address allowed as

(1) s/map/mapped/


> it doesn't need to be in the RAM, thus there's a call to update the
> page table.
> 
> The calculated frequency is only logged in this patch, it will be used
> in a following patch.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> 
> Notes:
>     v2:
>     - fix CamelCases
>     - Use U64 multiplication and division helpers
>     - don't read TscShift from the SharedInfo page again
>     - change the location of the shared info page to be outside of the ram
>     - check for overflow in XenDelay
>     - check for overflow when calculating the calculating APIC frequency
> 
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   2 +
>  OvmfPkg/XenPlatformPei/Platform.h         |   5 +
>  OvmfPkg/XenPlatformPei/Platform.c         |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c              | 177 ++++++++++++++++++++++
>  4 files changed, 185 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf 
> b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 8790d907d3ec..5732d2188871 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
>    DebugLib
>    HobLib
>    IoLib
> +  LocalApicLib
>    PciLib
>    ResourcePublicationLib
>    PeiServicesLib
> @@ -59,6 +60,7 @@ [LibraryClasses]
>    MtrrLib
>    MemEncryptSevLib
>    PcdLib
> +  SafeIntLib
>    XenHypercallLib
>  
>  [Pcd]
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h 
> b/OvmfPkg/XenPlatformPei/Platform.h
> index e70ca58078eb..77d88fc159d7 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -132,6 +132,11 @@ PhysicalAddressIdentityMapping (
>    IN EFI_PHYSICAL_ADDRESS AddressToMap
>    );
>  
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  );
> +
>  extern EFI_BOOT_MODE mBootMode;
>  
>  extern UINT8 mPhysMemAddressWidth;
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c 
> b/OvmfPkg/XenPlatformPei/Platform.c
> index 717fd0ab1a45..e9511eb40c62 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -448,6 +448,7 @@ InitializeXenPlatform (
>    InitializeRamRegions ();
>  
>    InitializeXen ();
> +  CalibrateLapicTimer ();
>  
>    if (mBootMode != BOOT_ON_S3_RESUME) {
>      ReserveEmuVariableNvStore ();
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index b2a7d1c21dac..7524aaa11a29 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -21,8 +21,10 @@
>  #include <Library/CpuLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
> +#include <Library/LocalApicLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/SafeIntLib.h>
>  #include <Guid/XenInfo.h>
>  #include <IndustryStandard/E820.h>
>  #include <Library/ResourcePublicationLib.h>
> @@ -457,3 +459,178 @@ PhysicalAddressIdentityMapping (
>  
>    return EFI_SUCCESS;
>  }
> +
> +STATIC
> +EFI_STATUS
> +MapSharedInfoPage (
> +  IN VOID *PagePtr
> +  )
> +{
> +  xen_add_to_physmap_t  Parameters;
> +  INTN                  ReturnCode;
> +
> +  Parameters.domid = DOMID_SELF;
> +  Parameters.space = XENMAPSPACE_shared_info;
> +  Parameters.idx = 0;
> +  Parameters.gpfn = (UINTN)PagePtr >> EFI_PAGE_SHIFT;
> +  ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
> +  if (ReturnCode != 0) {
> +    return EFI_NO_MAPPING;
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +VOID
> +UnmapXenPage (
> +  IN VOID *PagePtr
> +  )
> +{
> +  xen_remove_from_physmap_t Parameters;
> +  INTN                      ReturnCode;
> +
> +  Parameters.domid = DOMID_SELF;
> +  Parameters.gpfn = (UINTN)PagePtr >> EFI_PAGE_SHIFT;
> +  ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, 
> &Parameters);
> +  ASSERT (ReturnCode == 0);
> +}
> +
> +
> +STATIC
> +UINT64
> +GetCpuFreq (
> +  IN XEN_VCPU_TIME_INFO *VcpuTime
> +  )
> +{
> +  UINT32 Version;
> +  UINT32 TscToSystemMultiplier;
> +  INT8   TscShift;
> +  UINT64 CpuFreq;
> +
> +  do {
> +    Version = VcpuTime->Version;
> +    MemoryFence ();
> +    TscToSystemMultiplier = VcpuTime->TscToSystemMultiplier;
> +    TscShift = VcpuTime->TscShift;
> +    MemoryFence ();
> +  } while (((Version & 1) != 0) && (Version != VcpuTime->Version));
> +
> +  CpuFreq = DivU64x32 (LShiftU64 (1000000000ULL, 32), TscToSystemMultiplier);
> +  if (TscShift >= 0) {
> +      CpuFreq = RShiftU64 (CpuFreq, TscShift);
> +  } else {
> +      CpuFreq = LShiftU64 (CpuFreq, -TscShift);
> +  }
> +  return CpuFreq;
> +}
> +
> +STATIC
> +VOID
> +XenDelay (
> +  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
> +  IN UINT64             DelayNs
> +  )
> +{
> +  UINT64        Tick;
> +  UINT64        CpuFreq;
> +  UINT64        Delay;
> +  UINT64        DelayTick;
> +  UINT64        NewTick;
> +  RETURN_STATUS Status;
> +
> +  Tick = AsmReadTsc ();
> +
> +  CpuFreq = GetCpuFreq (VcpuTimeInfo);
> +  Status = SafeUint64Mult (DelayNs, CpuFreq, &Delay);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +        "XenDelay (%ld ns): delay too big in relation to CPU freq %ld Hz\n",
> +        DelayNs, CpuFreq));
> +    CpuDeadLoop ();
> +  }

(2) I suggest moving the ASSERT_EFI_ERROR() into the EFI_ERROR() branch,
namely between the DEBUG message and the CpuDeadLoop() call.

Applies to the rest of the ASSERT_EFI_ERROR() invocations in this patch.


(3) DelayNs and CpuFreq are of type UINT64, they should be logged with
%lu, not %ld.


(4) The indentation of the DEBUG macro arguments is incorrect; should be
2 spaces rather than 4.

Applies to the rest of the DEBUG() invocations in this patch.

> +
> +  DelayTick = DivU64x32 (Delay, 1000000000UL);

(5) The UL suffix on the constant is wasteful / misleading IMO;
DivU64x32 clearly takes a UINT32 as second parameter ("Divisor").


> +
> +  NewTick = Tick + DelayTick;
> +
> +  //
> +  // Check for overflow
> +  //
> +  if (NewTick < Tick) {
> +    //
> +    // Overflow, wait for TSC to also overflow
> +    //
> +    while (AsmReadTsc () >= Tick) {
> +      CpuPause ();
> +    }
> +  }
> +
> +  while (AsmReadTsc () <= NewTick) {
> +    CpuPause ();
> +  }
> +}
> +
> +
> +/**
> +  Calculate the frequency of the Local Apic Timer
> +**/
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  )
> +{
> +  XEN_SHARED_INFO       *SharedInfo;
> +  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
> +  UINT32                TimerTick, TimerTick2, DiffTimer;
> +  UINT64                TscTick, TscTick2;
> +  UINT64                Freq;
> +  UINT64                Dividend;
> +  EFI_STATUS            Status;
> +
> +
> +  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);

(I'm keeping in mind that this code is X64 only.)

> +  Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +        "Failed to add page table entry for Xen shared info page: %r\n",
> +        Status));
> +    return;
> +  }
> +
> +  Status = MapSharedInfoPage (SharedInfo);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to map Xen's shared info page: %r\n",
> +        Status));
> +    return;
> +  }
> +
> +  VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
> +
> +  InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
> +  DisableApicTimerInterrupt ();
> +
> +  TimerTick = GetApicTimerCurrentCount ();
> +  TscTick = AsmReadTsc ();
> +  XenDelay (VcpuTimeInfo, 1000000ULL);
> +  TimerTick2 = GetApicTimerCurrentCount ();
> +  TscTick2 = AsmReadTsc ();
> +
> +
> +  DiffTimer = TimerTick - TimerTick2;
> +  Status = SafeUint64Mult (GetCpuFreq (VcpuTimeInfo), DiffTimer, &Dividend);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "overflow while calculating APIC frequency\n"));
> +    DEBUG ((DEBUG_ERROR, "CPU freq: %ld Hz; APIC timer tick count for 1 ms: 
> %d\n",
> +        GetCpuFreq (VcpuTimeInfo), DiffTimer));

(6) Please use %lu and %u for formatting the UINT64 retval of
GetCpuFreq(), and the UINT32 variable DiffTimer, respectively.

All trivial feedback of course, so:

Acked-by: Laszlo Ersek <lersek@xxxxxxxxxx>

Thanks
Laszlo

> +    CpuDeadLoop ();
> +  }
> +
> +  Freq = DivU64x64Remainder (Dividend, TscTick2 - TscTick, NULL);
> +  DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
> +
> +  UnmapXenPage (SharedInfo);
> +}
>