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>
---
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);
+ 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);
+ Status = MapSharedInfoPage (SharedInfo);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
+
+ InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
+ DisableApicTimerInterrupt ();
+
+ TimerTick = GetApicTimerCurrentCount ();
+ TscTick = AsmReadTsc ();
+ XenDelay (VcpuTimeInfo, 1000000ULL);
+ TimerTick2 = GetApicTimerCurrentCount ();
+ TscTick2 = AsmReadTsc ();
+
+ Freq = (GetCPUFreq (VcpuTimeInfo) * (TimerTick - TimerTick2))
+ / (TscTick2 - TscTick);
+ DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
+
+ UnmapXenPage (SharedInfo);
+
+Exit:
+ FreePages (SharedInfo, 1);
+}
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53567): https://edk2.groups.io/g/devel/message/53567
Mute This Topic: https://groups.io/mt/70239983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, Jan 30, 2020 at 10:12:51AM +0100, Roger Pau Monné wrote: > On Wed, Jan 29, 2020 at 12:12:34PM +0000, Anthony PERARD wrote: > > + 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. I guess there's more refactoring to do in OVMF, there are other's of this kind of call, mostly in the PV drivers, XenBusDxe. > > + > > + 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. I'll look at what can be done with OVMF. Is there some kind of information that Xen could give? Or is it all information that OVMF should keep track of? Or if Xen only have XENMEM_memory_map, then OVMF already have this information. Thanks, -- Anthony PERARD -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53579): https://edk2.groups.io/g/devel/message/53579 Mute This Topic: https://groups.io/mt/70239983/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 30/01/2020 12:44, Anthony PERARD wrote: > On Thu, Jan 30, 2020 at 10:12:51AM +0100, Roger Pau Monné wrote: >> On Wed, Jan 29, 2020 at 12:12:34PM +0000, Anthony PERARD wrote: >>> + 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. > I guess there's more refactoring to do in OVMF, there are other's of > this kind of call, mostly in the PV drivers, XenBusDxe. > >>> + >>> + 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. > I'll look at what can be done with OVMF. > > Is there some kind of information that Xen could give? Or is it all > information that OVMF should keep track of? Or if Xen only have > XENMEM_memory_map, then OVMF already have this information. We need to actually tackle the memory problem, and provide something correct via XENMEM_memory_map (/similar). So far, noone has actually started to try fixing the problem. Perhaps now is a good enough kick to get started. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 01/29/20 13:12, 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>
> ---
> 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(+)
I'll review this superficially; it should be approved by someone from
xen-devel:
> 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;
(1) Please remove the space character from "(UINTN) PagePtr". Inserting
a space character is a bad and confusing habit in edk2, because it masks
the fact that the cast operator has one of the strongest bindings in the
C language. So I try to keep it out of OvmfPkg and ArmVirtPkg.
> + 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;
(2) Ditto.
> + 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 = (1000000000ULL << 32) / TSCToSystemMultiplier;
(3) I understand that OvmfXen is X64, and so this code will not be built
for IA32 in practice. Still, for sticking with the coding style, it's
better to use LShiftU64() here, and then DivU64x32().
> + if (TSCShift >= 0) {
> + CPUFreq >>= VcpuTime->TSCShift;
> + } else {
> + CPUFreq <<= -VcpuTime->TSCShift;
> + }
(4) Please use LShiftU64() and RShiftU64().
(5) I think there's a typo here: you just fished out "TscShift" from the
shared info page; we should used that cached value, and not access the
shared info page again. Is that right?
> + return CPUFreq;
> +}
> +
> +VOID
> +XenDelay (
> + IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
> + IN UINT64 DelayNs
> + )
> +{
> + UINT64 Tick;
> +
> + Tick = AsmReadTsc ();
> + Tick += (DelayNs * GetCPUFreq (VcpuTimeInfo)) / 1000000000ULL;
(6) Please use MultU64x64() and DivU64x32(). (1,000,000,000 fits in a
UINT32.)
> + 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);
(7) Can you check if this succeeds?
> + Status = MapSharedInfoPage (SharedInfo);
> + ASSERT_EFI_ERROR (Status);
> + if (EFI_ERROR (Status)) {
> + goto Exit;
> + }
> +
> + VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
> +
> + InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
> + DisableApicTimerInterrupt ();
> +
> + TimerTick = GetApicTimerCurrentCount ();
> + TscTick = AsmReadTsc ();
> + XenDelay (VcpuTimeInfo, 1000000ULL);
> + TimerTick2 = GetApicTimerCurrentCount ();
> + TscTick2 = AsmReadTsc ();
> +
> + Freq = (GetCPUFreq (VcpuTimeInfo) * (TimerTick - TimerTick2))
> + / (TscTick2 - TscTick);
(8) Please use the above-mentioned U64 multiplication and division helpers.
(9) In case we are concerned about U64 overflows anywhere in this patch:
SafeIntLib has range-checked functions, for example SafeUint64Mult().
Thanks!
Laszlo
> + DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
> +
> + UnmapXenPage (SharedInfo);
> +
> +Exit:
> + FreePages (SharedInfo, 1);
> +}
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53529): https://edk2.groups.io/g/devel/message/53529
Mute This Topic: https://groups.io/mt/70239983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.