From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, sunilvl@ventanamicro.com
Cc: Gerd Hoffmann <kraxel@redhat.com>,
Rahul Kumar <rahul1.kumar@intel.com>, Ray Ni <ray.ni@intel.com>,
Andrei Warkentin <andrei.warkentin@intel.com>
Subject: Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
Date: Thu, 4 Jan 2024 15:38:17 +0100 [thread overview]
Message-ID: <ce313c72-fe8b-cc12-13ad-afc2b2bb7d85@redhat.com> (raw)
In-Reply-To: <20240103135849.127251-4-sunilvl@ventanamicro.com>
On 1/3/24 14:58, Sunil V L wrote:
> Sstc extension allows to program the timer and receive the interrupt
> without using an SBI call. This reduces the latency to generate the timer
> interrupt. So, detect whether Sstc extension is supported and use the
> stimecmp register directly to program the timer interrupt.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
> .../CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf | 1 +
> UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h | 2 ++
> UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 30 +++++++++++++++++--
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
> index aba660186dc0..f2a2cf12caef 100644
> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
> @@ -41,6 +41,7 @@ [Sources.RISCV64]
> Timer.c
>
> [Pcd]
> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency ## CONSUMES
>
> [Protocols]
> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
> index 9b3542230cb5..5e5071b3f0b2 100644
> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
> @@ -26,6 +26,8 @@
> //
> #define DEFAULT_TIMER_TICK_DURATION 100000
>
> +#define RISCV_CPU_FEATURE_SSTC_BITMASK 0x2
(1) Not a bug by any means, but BIT1 might read more idiomatic.
> +
> extern VOID
> RiscvSetTimerPeriod (
> UINT32 TimerPeriod
> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> index 30e48061cd06..4babfb4bfc60 100644
> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> @@ -44,6 +44,19 @@ STATIC EFI_TIMER_NOTIFY mTimerNotifyFunction;
> STATIC UINT64 mTimerPeriod = 0;
> STATIC UINT64 mLastPeriodStart = 0;
>
> +/**
> + Check whether Sstc is enabled in PCD.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsSstcEnabled (
> + VOID
> + )
> +{
> + return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_SSTC_BITMASK) != 0);
> +}
> +
> /**
> Timer Interrupt Handler.
>
> @@ -94,7 +107,12 @@ TimerInterruptHandler (
> ),
> 1000000u
> ); // convert to tick
> - SbiSetTimer (PeriodStart);
> + if (RiscVIsSstcEnabled ()) {
(2) Even though the PCD is currently declared as fixed or
patchable-in-module, seeing a PcdGet64() call on the call stack of the
timer interrupt handler (and at a high TPL) makes me uncomfortable. It
carries a risk that later on we relax the PCD decl to dynamic, and then
this code would become brittle.
I propose: either replace the PcdGet64 call above with FixedPcdGet64 (so
it can never land in the runtime / dynamic PCD protocol), or perform the
PCD check in the entry point function of the driver, and store the
result in a STATIC BOOLEAN variable. Then further PCD accesses (dynamic
or otherwise) will not be needed.
> + RiscVSetSupervisorTimeCompareRegister (PeriodStart);
> + } else {
> + SbiSetTimer (PeriodStart);
> + }
> +
> RiscVEnableTimerInterrupt (); // enable SMode timer int
> gBS->RestoreTPL (OriginalTPL);
> }
> @@ -197,7 +215,11 @@ TimerDriverSetTimerPeriod (
> ),
> 1000000u
> ); // convert to tick
> - SbiSetTimer (PeriodStart);
> + if (RiscVIsSstcEnabled ()) {
> + RiscVSetSupervisorTimeCompareRegister (PeriodStart);
> + } else {
> + SbiSetTimer (PeriodStart);
> + }
>
> mCpu->EnableInterrupt (mCpu);
> RiscVEnableTimerInterrupt (); // enable SMode timer int
(3) This seems like duplicated code. How about replacing the
RiscVIsSstcEnabled() function with a more substantive function that
incorporates both the feature check *and* the "PeriodStart" setting?
Then you can easily call that function from both TimerInterruptHandler()
and TimerDriverSetTimerPeriod().
> @@ -282,6 +304,10 @@ TimerDriverInitialize (
> //
> mTimerNotifyFunction = NULL;
>
> + if (RiscVIsSstcEnabled ()) {
> + DEBUG ((DEBUG_INFO, "%a: Timer interrupt is via Sstc extension\n", __func__));
> + }
> +
Right, this would be the place to fetch the PCD explicitly and to store
the result (based on bit-masking) into the global boolean.
> //
> // Make sure the Timer Architectural Protocol is not already installed in the system
> //
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113176): https://edk2.groups.io/g/devel/message/113176
Mute This Topic: https://groups.io/mt/103501843/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-01-04 14:38 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 13:58 [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension Sunil V L
2024-01-03 13:58 ` [edk2-devel] [PATCH 1/4] MdePkg.dec: RISC-V: Define override bit " Sunil V L
2024-01-03 13:58 ` [edk2-devel] [PATCH 2/4] MdePkg/BaseLib: RISC-V: Add function to update stimecmp register Sunil V L
2024-01-03 13:58 ` [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc Sunil V L
2024-01-04 14:38 ` Laszlo Ersek [this message]
2024-01-04 15:01 ` Sunil V L
2024-01-05 13:52 ` Laszlo Ersek
2024-01-04 15:46 ` Sunil V L
2024-01-05 13:52 ` Laszlo Ersek
2024-01-03 13:58 ` [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension Sunil V L
2024-01-04 14:32 ` Laszlo Ersek
2024-01-04 14:38 ` Laszlo Ersek
2024-01-05 19:10 ` [edk2-devel] [PATCH 0/4] RISC-V: Add support for " Pedro Falcato
2024-01-08 4:06 ` Sunil V L
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ce313c72-fe8b-cc12-13ad-afc2b2bb7d85@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox