Re: [PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Jan 29, 2020 at 12:12:34PM +0000, Anthony PERARD 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 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@citrix.com>

Thanks! Some comments below on the implementation.

> ---
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   1 +
>  OvmfPkg/XenPlatformPei/Platform.h         |   5 +
>  OvmfPkg/XenPlatformPei/Platform.c         |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c              | 123 ++++++++++++++++++++++
>  4 files changed, 130 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 0ef77db92c03..335a442538c2 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
>    DebugLib
>    HobLib
>    IoLib
> +  LocalApicLib
>    PciLib
>    ResourcePublicationLib
>    PeiServicesLib
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 7661f4a8de0a..97e482a065f0 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -127,6 +127,11 @@ XenGetE820Map (
>    UINT32 *Count
>    );
>  
> +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 c41fecdc486e..d6cdc9a8e31c 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -19,6 +19,7 @@
>  //
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
> +#include <Library/LocalApicLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
>  #include <Guid/XenInfo.h>
> @@ -386,3 +387,125 @@ InitializeXen (
>  
>    return EFI_SUCCESS;
>  }
> +
> +
> +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;
> +}
> +
> +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);

I'm afraid this will leave a hole in the p2m, and hence freeing the
page back to OVMF is wrong (I assume this is what FreePages does in
CalibrateLapicTimer), as the physical address would be unpopulated
after the call to XENMEM_remove_from_physmap.

> +  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 = (1000000000ULL << 32) / TSCToSystemMultiplier;
> +  if (TSCShift >= 0) {
> +      CPUFreq >>= VcpuTime->TSCShift;
> +  } else {
> +      CPUFreq <<= -VcpuTime->TSCShift;
> +  }
> +  return CPUFreq;
> +}
> +
> +VOID
> +XenDelay (
> +  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
> +  IN UINT64             DelayNs
> +  )
> +{
> +  UINT64 Tick;
> +
> +  Tick = AsmReadTsc ();
> +  Tick += (DelayNs * GetCPUFreq (VcpuTimeInfo)) / 1000000000ULL;
> +  while (AsmReadTsc() <= Tick) {
> +    CpuPause();
> +  }
> +}
> +
> +
> +/**
> +  Calculate the frequency of the Local Apic Timer
> +**/
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  )
> +{
> +  XEN_SHARED_INFO       *SharedInfo;
> +  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
> +  UINT32                TimerTick, TimerTick2;
> +  UINT64                TscTick, TscTick2;
> +  UINT64                Freq;
> +  EFI_STATUS            Status;
> +
> +  SharedInfo = AllocatePages (1);

Hm, it's not the best approach to use a regular memory page to map the
shared info: mapping it is very likely to cause superpage shattering,
and once unmapped it leaves a hole in the p2m.

As a reference you could map the shared info page at maximum physical
address allowed (after checking it's not populated) like Wei is doing
for the Xen on HyperV work.

Roger.