From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web09.7812.1580315748462984646 for ; Wed, 29 Jan 2020 08:35:48 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=I5j+Snht; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580315747; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6+NG/EfzaLh1hbcguaElD/gu52IG31ucl1DGI1XoZy8=; b=I5j+SnhtQ4xnaiW8+rimU9jZWahFNKrZvQ3AiUJO7gXySQIRsA3xxMcE+Ue7sw+y8XG3Gx a1zvK5EZRgSJ/fp3VNh4Pil6k3Owg/8GD2zNQn/kvArPT3yriNHBDDNSXmANp76Zu6Tsom QyJvk2fKHzIR56+M6z16S4dYdShEt/M= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-420-aY4o0wdpOp-u3ePYdYY8IA-1; Wed, 29 Jan 2020 11:35:31 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EF541108442F; Wed, 29 Jan 2020 16:35:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-32.ams2.redhat.com [10.36.116.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6D77387B0D; Wed, 29 Jan 2020 16:29:59 +0000 (UTC) Subject: Re: [PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency To: Anthony PERARD , devel@edk2.groups.io Cc: Michael D Kinney , Ard Biesheuvel , xen-devel@lists.xenproject.org, Liming Gao , Jordan Justen , Julien Grall , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= References: <20200129121235.1814563-1-anthony.perard@citrix.com> <20200129121235.1814563-5-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <4060e55f-20bf-38a4-6586-3abdcde91836@redhat.com> Date: Wed, 29 Jan 2020 17:29:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200129121235.1814563-5-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: aY4o0wdpOp-u3ePYdYY8IA-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/29/20 13:12, Anthony PERARD wrote: > Calculate the frequency of the APIC timer that Xen provides. >=20 > 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. >=20 > 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). >=20 > The calculated frequency is only logged in this patch, it will be used > in a following patch. >=20 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2490 > Signed-off-by: Anthony PERARD > --- > CC: Roger Pau Monn=C3=A9 > --- > 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/XenPlatf= ormPei/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/P= latform.h > index 7661f4a8de0a..97e482a065f0 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.h > +++ b/OvmfPkg/XenPlatformPei/Platform.h > @@ -127,6 +127,11 @@ XenGetE820Map ( > UINT32 *Count > ); > =20 > +VOID > +CalibrateLapicTimer ( > + VOID > + ); > + > extern EFI_BOOT_MODE mBootMode; > =20 > extern UINT8 mPhysMemAddressWidth; > diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/P= latform.c > index 717fd0ab1a45..e9511eb40c62 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.c > +++ b/OvmfPkg/XenPlatformPei/Platform.c > @@ -448,6 +448,7 @@ InitializeXenPlatform ( > InitializeRamRegions (); > =20 > InitializeXen (); > + CalibrateLapicTimer (); > =20 > if (mBootMode !=3D 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 > #include > +#include > #include > #include > #include > @@ -386,3 +387,125 @@ InitializeXen ( > =20 > return EFI_SUCCESS; > } > + > + > +EFI_STATUS > +MapSharedInfoPage ( > + IN VOID *PagePtr > + ) > +{ > + xen_add_to_physmap_t Parameters; > + INTN ReturnCode; > + > + Parameters.domid =3D DOMID_SELF; > + Parameters.space =3D XENMAPSPACE_shared_info; > + Parameters.idx =3D 0; > + Parameters.gpfn =3D (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 =3D XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameter= s); > + if (ReturnCode !=3D 0) { > + return EFI_NO_MAPPING; > + } > + return EFI_SUCCESS; > +} > + > +VOID > +UnmapXenPage ( > + IN VOID *PagePtr > + ) > +{ > + xen_remove_from_physmap_t Parameters; > + INTN ReturnCode; > + > + Parameters.domid =3D DOMID_SELF; > + Parameters.gpfn =3D (UINTN) PagePtr >> EFI_PAGE_SHIFT; (2) Ditto. > + ReturnCode =3D XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Para= meters); > + ASSERT (ReturnCode =3D=3D 0); > +} > + > + > +STATIC > +UINT64 > +GetCPUFreq ( > + IN XEN_VCPU_TIME_INFO *VcpuTime > + ) > +{ > + UINT32 Version; > + UINT32 TSCToSystemMultiplier; > + INT8 TSCShift; > + UINT64 CPUFreq; > + > + do { > + Version =3D VcpuTime->Version; > + MemoryFence (); > + TSCToSystemMultiplier =3D VcpuTime->TSCToSystemMultiplier; > + TSCShift =3D VcpuTime->TSCShift; > + MemoryFence (); > + } while (((Version & 1) !=3D 0) && (Version !=3D VcpuTime->Version)); > + > + CPUFreq =3D (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 >=3D 0) { > + CPUFreq >>=3D VcpuTime->TSCShift; > + } else { > + CPUFreq <<=3D -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 =3D AsmReadTsc (); > + Tick +=3D (DelayNs * GetCPUFreq (VcpuTimeInfo)) / 1000000000ULL; (6) Please use MultU64x64() and DivU64x32(). (1,000,000,000 fits in a UINT32.) > + while (AsmReadTsc() <=3D 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 =3D AllocatePages (1); (7) Can you check if this succeeds? > + Status =3D MapSharedInfoPage (SharedInfo); > + ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + goto Exit; > + } > + > + VcpuTimeInfo =3D &SharedInfo->VcpuInfo[0].Time; > + > + InitializeApicTimer (1, MAX_UINT32, TRUE, 0); > + DisableApicTimerInterrupt (); > + > + TimerTick =3D GetApicTimerCurrentCount (); > + TscTick =3D AsmReadTsc (); > + XenDelay (VcpuTimeInfo, 1000000ULL); > + TimerTick2 =3D GetApicTimerCurrentCount (); > + TscTick2 =3D AsmReadTsc (); > + > + Freq =3D (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); > +} >=20