From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7C860AC18F8 for ; Mon, 11 Dec 2023 17:16:37 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=cWmS6F5SZFigzkpAWeB345TXAWmUATWCtHc0QffUpIE=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1702314995; v=1; b=iSS3ZZyMhbq9Vbh7uDLnz38d55FnJUV9YBE0Dl5syJGI0mzXZFKWnd7LMMlOb1JVxFJ3HOqZ eAtCPoGuX9oRiAx2SUYAd/f6wuTJlzmDx3DV8J9gT+dEx4Y6BI5eV8c+0l+zOokiOE8fB34VOTy FBGgtrWbDZGO75kLlSoDA2KM= X-Received: by 127.0.0.2 with SMTP id UfdqYY7687511xS90h5EMEcH; Mon, 11 Dec 2023 09:16:35 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.755.1702314995064585111 for ; Mon, 11 Dec 2023 09:16:35 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-144-hFmyDtnFOvKWfBdqyspVVA-1; Mon, 11 Dec 2023 12:16:29 -0500 X-MC-Unique: hFmyDtnFOvKWfBdqyspVVA-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 970F33811F37; Mon, 11 Dec 2023 17:16:28 +0000 (UTC) X-Received: from [10.39.194.43] (unknown [10.39.194.43]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BB10B3C27; Mon, 11 Dec 2023 17:16:26 +0000 (UTC) Message-ID: <8c11be85-051e-382e-14a2-736d7565fb54@redhat.com> Date: Mon, 11 Dec 2023 18:16:20 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 11/39] UefiCpuPkg: Add LoongArch64 CPU Timer library To: devel@edk2.groups.io, lichao@loongson.cn Cc: Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20231117095742.3605778-1-lichao@loongs> <20231117100007.3609080-1-lichao@loongson.cn> <2e42db7c-a927-f47b-7d80-632895b11e1b@redhat.com> <1069d005-ef80-46c6-89fa-f3fed316e4ad@loongson.cn> From: "Laszlo Ersek" In-Reply-To: <1069d005-ef80-46c6-89fa-f3fed316e4ad@loongson.cn> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ZLBnxqPhkXlugSZFLjqDA7Cqx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=iSS3ZZyM; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/23/23 12:43, Chao Li wrote: > On 2023/11/23 00:12, Laszlo Ersek wrote: >> On 11/17/23 11:00, Chao Li wrote: >>> Add the LoongArch64 CPU Timer library, using CPUCFG 0x4 and 0x5 for >>> Stable Counter frequency. >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4584 >>> >>> Cc: Eric Dong >>> Cc: Ray Ni >>> Cc: Rahul Kumar >>> Cc: Gerd Hoffmann >>> Signed-off-by: Chao Li >>> --- >>> .../BaseLoongArch64CpuTimerLib.inf | 30 +++ >>> .../BaseLoongArch64CpuTimerLib.uni | 15 ++ >>> .../BaseLoongArch64CpuTimerLib/CpuTimerLib.c | 226 ++++++++++++++++++ >>> UefiCpuPkg/UefiCpuPkg.dsc | 3 + >>> 4 files changed, 274 insertions(+) >>> create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseL= oongArch64CpuTimerLib.inf >>> create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseL= oongArch64CpuTimerLib.uni >>> create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTi= merLib.c >> (1) sorry about the annoying comment, but the library should be >> called (preferably) BaseTimerLibLoongArch64Cpu. >> >> We're frequently not consistent with the following library instance >> naming scheme, but in theory anyway, library instances should be >> named as follows: >> >> >> >> Thus, in this case, Base + TimerLib + LoongArch64Cpu. >> >> update UNI file name, INF file name, directory name, BASE_NAME and >> MODULE_UNI_FILE accordingly (if you agree) > > There has a SPEC for naming style: > > https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/blob= /master/4_naming_conventions/42_directory_names.md > > Please check 4.2.3 EDKII Library directory, and most directory naming > follows this SPEC. I didn't remember (or know about) this part of the coding standards spec; thanks for the pointer. It seems like my suggestion matches the second alternative ([]). Your naming seems to follow the first alternative ([]). OK. However, that's still not a perfect match. For , you have "Base". For , you have "LoongArch64". But for , you have "CpuTimerLib", and that's wrong. We don't have a "CpuTimerLib" class; instead it's the "TimerLib" class. The LIBRARY_CLASS define in your INF file also says "TimerLib", so "CpuTimerLib" is wrong / inconsistent in the lib instance name. "BaseLoongArch64TimerLib" should be fine. If you want to add "Cpu", I guess you could add it as [], as a suffix, under the first naming scheme alternative: "BaseLoongArch64TimerLibCpu". >> (5) Relatedly: the TimerLib class is declared in "MdePkg.dec". Thus, >> if you indeed don't need anything from "UefiCpuPkg.dec", I'd suggest >> moving this library instance under MdePkg. > It is inappropriate to place this library in MdePkg, because the > MdePkg doesn't have any CpuTimerLib instance, and this class library > are HW implementation-related, so it more appropriate to place it in > UefiCpuPkg. This ties in with my above comment. The reason for MdePkg not having a "CpuTimerLib" instance is that there is no such lib class in edk2. The library class is called "TimerLib". We have multiple instances in edk2: $ git grep -l 'LIBRARY_CLASS *=3D *TimerLib\>' -- '*inf' ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.inf EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.inf MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLibBhyve.inf OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/StandaloneMmAcpiTimerLib.inf UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/BaseRiscV64CpuTimerLib.inf UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf This "prior art" suggests the new library instance could go in MdePkg or UefiCpuPkg alike. That's where my comment about "UefiCpuPkg.dec" dependencies becomes relevant. Compare: - MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf - UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf The difference between these library instances (as the INF files explain) is xAPIC vs. x2APIC support. The latter instance supports x2APIC. And, for x2APIC support, the latter instance depends on the LocalApicLib class. The LocalApicLib class is declared in "UefiCpuPkg.dec". And that is the dependency that forces the second library instance to exist under UefiCpuPkg. Thus, my original point stands -- if you need nothing from "UefiCpuPkg", then your library instance "BaseLoongArch64TimerLib" (or "BaseLoongArch64TimerLibCpu", if you like) should go under MdePkg, in my opinion anyway. (A different example: "UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf" lives under UefiCpuPkg due to depending on "PcdCpuCoreCrystalClockFrequency", which also comes from "UefiCpuPkg.dec".) >>> + VOID >>> + ) >>> +{ >>> + UINT32 BaseFreq; >>> + UINT32 ClockMultiplier; >>> + UINT32 ClockDivide; >>> + CPUCFG_REG4_INFO_DATA CCFreq; >> (10) Per edk2 coding style, when acronyms and abbreviations are grouped >> together, each individual acronym itself is supposed to be CamelCase. >> For example, when we use "PCI" in identifiers, it becomes "Pci". So I >> think this variable should be called "CcFreq", where "CC" stands for >> "CPU Config". If I'm wrong about "CC", then please ignore this point. > The two letter 'C' means: first 'C' means Constant, and the second means > Crystal. In LoongArch SPEC, this area is named CC_FREQ, which means: > Constant frequency timer and the crystal frequency corresponding to the > clock used by the timer. So I think CCFreq is more suitable. "PCI" is also an acronym ("Peripheral Component Interconnect"), so "CC" for "Constant Crystal" is no different, from this perspective. The same way we write PciRead8(), the variable should be CcFreq. >>> + CPUCFG_REG5_INFO_DATA CpucfgReg5Data; >>> + UINT32 StableTimerFreq; >>> + >>> + // >>> + // Get the the crystal frequency corresponding to the constant >>> + // frequency timer and the clock used by the timer. >>> + // >>> + AsmCpucfg (CPUCFG_REG4_INFO, &CCFreq.Uint32); >>> + >>> + // >>> + // Get the multiplication factor and frequency division factor >>> + // corresponding to the constant frequency timer and the clock >>> + // used by the timer. >>> + // >>> + AsmCpucfg (CPUCFG_REG5_INFO, &CpucfgReg5Data.Uint32); >>> + >>> + BaseFreq =3D CCFreq.Bits.CC_FREQ; >> (11) My comment here probably affects the header file where you >> introduced the CC_FREQ field name. It should be CcFreq. >> >> Same for MUL and DIV below. > The same to above. I understand the LoongArch spec may call this "CC_FREQ", but that's just not how edk2 spells field names. "CC_FREQ" is not idiomatic, the field too should be called "CcFreq". Anyway, I don't insist, just wanted to make you aware. >>> + ASSERT (0); >> (15) Should be ASSERT (FALSE). >> >> (16) I recommend that a CpuDeadLoop() call be added here too, because >> ASSERT()s are compiled out of RELEASE builds. > For double (13), (14), (15), (16) and (17), I agree, will fix them in > V4. But for 15, did the CpuDeadLoop() be needed? I think if the DEBUG > mode doesn't work, the RELEASE mode can not be created. You are not asserting an algorithmic invariant, but an environmental circumstance (namely that the clock does not show unreasonable properties). Meaning that the predicate can be falsified due to environmental reasons, even if there is no programming or design error in the code. Therefore, in my opinion, such *environment* checks are best preserved in RELEASE builds as well. Either way, up to you. >> (25) This proves that, generally speaking, the final "+=3D" operation >> (increment) is unsafe -- in every TimerLib instance in edk2 that uses >> this calculation --, and should be replaced with an addition from >> SafeIntLib. > Do you recommend changing the final "+=3D" to the SafeIntLib way? > Something like: NanoSeconds =3D SafeUint64Add (NanoSeconds, > DivU64x64Remainder (MultU64x32 (Remainder, 1000000000u), Frequency, > NULL), &NanoSeconds). Something like that, yes, please. While at it, using some helper variables could improve readability, plus AFAIR, SafeUint64Add() returns a RETURN_STATUS vaule. ... sorry about the late followup. I've been out sick for two weeks, and have returned now for only a few days again, before the year ends. I think it unlikely that I can continue reviewing v3 in any timely manner, so feel free to post v4 whenever you're ready to do so (assuming no-one else wants to continue reviewing v3). Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112320): https://edk2.groups.io/g/devel/message/112320 Mute This Topic: https://groups.io/mt/102644766/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-