* [edk2-devel] [PATCH 1/4] MdePkg.dec: RISC-V: Define override bit for Sstc extension
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 ` 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
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Sunil V L @ 2024-01-03 13:58 UTC (permalink / raw)
To: devel
Cc: Sunil V L, Liming Gao, Michael D Kinney, Zhiguang Liu,
Andrei Warkentin
Define the BIT 1 as the override bit for Sstc extension. This will be
used by the timer driver to decide whether to use SBI calls or direct
CSR access to configure the timer.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
MdePkg/MdePkg.dec | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 2ee112cc087a..0459418906f8 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2405,6 +2405,8 @@ [PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
# Configurability to override RISC-V CPU Features
# BIT 0 = Cache Management Operations. This bit is relevant only if
# previous stage has feature enabled and user wants to disable it.
+ # BIT 1 = Supervisor Time Compare (Sstc). This bit is relevant only if
+ # previous stage has feature enabled and user wants to disable it.
#
gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113093): https://edk2.groups.io/g/devel/message/113093
Mute This Topic: https://groups.io/mt/103501838/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 14+ messages in thread* [edk2-devel] [PATCH 2/4] MdePkg/BaseLib: RISC-V: Add function to update stimecmp register
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 ` Sunil V L
2024-01-03 13:58 ` [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc Sunil V L
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Sunil V L @ 2024-01-03 13:58 UTC (permalink / raw)
To: devel
Cc: Sunil V L, Michael D Kinney, Liming Gao, Zhiguang Liu,
Andrei Warkentin
stimecmp is a CSR supported only when Sstc extension is supported by the
platform. This register can be used to set the timer interrupt directly in
S-mode instead of going via SBI call. Add a function to update this
register.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
MdePkg/Include/Library/BaseLib.h | 5 +++++
MdePkg/Include/Register/RiscV64/RiscVEncoding.h | 3 +++
MdePkg/Library/BaseLib/RiscV64/ReadTimer.S | 7 +++++++
3 files changed, 15 insertions(+)
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index b71e47f41b7f..ca0d06c7f335 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -191,6 +191,11 @@ RiscVReadTimer (
VOID
);
+VOID
+RiscVSetSupervisorTimeCompareRegister (
+ IN UINT64
+ );
+
VOID
RiscVEnableTimerInterrupt (
VOID
diff --git a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
index 2bde8db478ff..8ccdea2f4fcd 100644
--- a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
+++ b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
@@ -96,6 +96,9 @@
/* Supervisor Protection and Translation */
#define CSR_SATP 0x180
+/* Sstc extension */
+#define CSR_STIMECMP 0x14D
+
/* Trap/Exception Causes */
#define CAUSE_MISALIGNED_FETCH 0x0
#define CAUSE_FETCH_ACCESS 0x1
diff --git a/MdePkg/Library/BaseLib/RiscV64/ReadTimer.S b/MdePkg/Library/BaseLib/RiscV64/ReadTimer.S
index 39a06efa51ef..36781c29c0b9 100644
--- a/MdePkg/Library/BaseLib/RiscV64/ReadTimer.S
+++ b/MdePkg/Library/BaseLib/RiscV64/ReadTimer.S
@@ -21,3 +21,10 @@
ASM_FUNC (RiscVReadTimer)
csrr a0, CSR_TIME
ret
+
+//
+// Set Supervisor Time Compare Register
+//
+ASM_FUNC (RiscVSetSupervisorTimeCompareRegister)
+ csrw CSR_STIMECMP, a0
+ ret
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113094): https://edk2.groups.io/g/devel/message/113094
Mute This Topic: https://groups.io/mt/103501841/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 14+ messages in thread* [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
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 ` Sunil V L
2024-01-04 14:38 ` Laszlo Ersek
2024-01-03 13:58 ` [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension Sunil V L
2024-01-05 19:10 ` [edk2-devel] [PATCH 0/4] RISC-V: Add support for " Pedro Falcato
4 siblings, 1 reply; 14+ messages in thread
From: Sunil V L @ 2024-01-03 13:58 UTC (permalink / raw)
To: devel
Cc: Sunil V L, Gerd Hoffmann, Rahul Kumar, Laszlo Ersek, Ray Ni,
Andrei Warkentin
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
+
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 ()) {
+ 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
@@ -282,6 +304,10 @@ TimerDriverInitialize (
//
mTimerNotifyFunction = NULL;
+ if (RiscVIsSstcEnabled ()) {
+ DEBUG ((DEBUG_INFO, "%a: Timer interrupt is via Sstc extension\n", __func__));
+ }
+
//
// Make sure the Timer Architectural Protocol is not already installed in the system
//
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113095): https://edk2.groups.io/g/devel/message/113095
Mute This Topic: https://groups.io/mt/103501843/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
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
2024-01-04 15:01 ` Sunil V L
2024-01-04 15:46 ` Sunil V L
0 siblings, 2 replies; 14+ messages in thread
From: Laszlo Ersek @ 2024-01-04 14:38 UTC (permalink / raw)
To: devel, sunilvl; +Cc: Gerd Hoffmann, Rahul Kumar, Ray Ni, Andrei Warkentin
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
2024-01-04 14:38 ` Laszlo Ersek
@ 2024-01-04 15:01 ` Sunil V L
2024-01-05 13:52 ` Laszlo Ersek
2024-01-04 15:46 ` Sunil V L
1 sibling, 1 reply; 14+ messages in thread
From: Sunil V L @ 2024-01-04 15:01 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: devel, Gerd Hoffmann, Rahul Kumar, Ray Ni, Andrei Warkentin
Hi Laszlo,
Thank you very much for the review!.
On Thu, Jan 04, 2024 at 03:38:17PM +0100, Laszlo Ersek wrote:
> 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.
>
Agree. Would RISCV_CPU_FEATURE_BIT1_SSTC be better?
> > +
> > 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.
>
Ahh yes. Good point. Let me use a static variable as you suggested.
> > + 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().
>
I agree. Let me update in the next version.
> > @@ -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.
>
Yes!
Thanks!
Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113181): https://edk2.groups.io/g/devel/message/113181
Mute This Topic: https://groups.io/mt/103501843/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
2024-01-04 15:01 ` Sunil V L
@ 2024-01-05 13:52 ` Laszlo Ersek
0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2024-01-05 13:52 UTC (permalink / raw)
To: Sunil V L; +Cc: devel, Gerd Hoffmann, Rahul Kumar, Ray Ni, Andrei Warkentin
On 1/4/24 16:01, Sunil V L wrote:
> Hi Laszlo,
>
> Thank you very much for the review!.
>
> On Thu, Jan 04, 2024 at 03:38:17PM +0100, Laszlo Ersek wrote:
>> 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.
>>
> Agree. Would RISCV_CPU_FEATURE_BIT1_SSTC be better?
Sorry, I was unclear: the macro *name* was fine, IMO; my proposal was to
change the *replacement text* from 0x2 to BIT1. (Because BIT1 is another
macro, from "MdePkg/Include/Base.h"; those are frequently used all over
edk2.)
Thanks!
Laszlo
>
>>> +
>>> 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.
>>
> Ahh yes. Good point. Let me use a static variable as you suggested.
>
>>> + 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().
>>
> I agree. Let me update in the next version.
>
>>> @@ -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.
>>
> Yes!
>
> Thanks!
> Sunil
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113284): https://edk2.groups.io/g/devel/message/113284
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
2024-01-04 14:38 ` Laszlo Ersek
2024-01-04 15:01 ` Sunil V L
@ 2024-01-04 15:46 ` Sunil V L
2024-01-05 13:52 ` Laszlo Ersek
1 sibling, 1 reply; 14+ messages in thread
From: Sunil V L @ 2024-01-04 15:46 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: devel, Gerd Hoffmann, Rahul Kumar, Ray Ni, Andrei Warkentin
On Thu, Jan 04, 2024 at 03:38:17PM +0100, Laszlo Ersek wrote:
> 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.
>
I misunderstood your comment. Will use BIT1 instead of 0x2.
Thanks!
Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113184): https://edk2.groups.io/g/devel/message/113184
Mute This Topic: https://groups.io/mt/103501843/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
2024-01-04 15:46 ` Sunil V L
@ 2024-01-05 13:52 ` Laszlo Ersek
0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2024-01-05 13:52 UTC (permalink / raw)
To: Sunil V L; +Cc: devel, Gerd Hoffmann, Rahul Kumar, Ray Ni, Andrei Warkentin
On 1/4/24 16:46, Sunil V L wrote:
> On Thu, Jan 04, 2024 at 03:38:17PM +0100, Laszlo Ersek wrote:
>> 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.
>>
> I misunderstood your comment. Will use BIT1 instead of 0x2.
OK then :)
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113285): https://edk2.groups.io/g/devel/message/113285
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension
2024-01-03 13:58 [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension Sunil V L
` (2 preceding siblings ...)
2024-01-03 13:58 ` [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc Sunil V L
@ 2024-01-03 13:58 ` 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
4 siblings, 2 replies; 14+ messages in thread
From: Sunil V L @ 2024-01-03 13:58 UTC (permalink / raw)
To: devel
Cc: Sunil V L, Andrei Warkentin, Ard Biesheuvel, Gerd Hoffmann,
Jiewen Yao, Laszlo Ersek
Override Sstc extension and use SBI calls itself by default for RISC-V
qemu virt platform.
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index d3624e899e8d..6bc7c90f31dc 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -203,7 +203,7 @@ [PcdsFeatureFlag]
gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
[PcdsFixedAtBuild.common]
- gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFE
+ gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFC
gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113096): https://edk2.groups.io/g/devel/message/113096
Mute This Topic: https://groups.io/mt/103501846/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension
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
1 sibling, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2024-01-04 14:32 UTC (permalink / raw)
To: devel, sunilvl
Cc: Andrei Warkentin, Ard Biesheuvel, Gerd Hoffmann, Jiewen Yao
On 1/3/24 14:58, Sunil V L wrote:
> Override Sstc extension and use SBI calls itself by default for RISC-V
> qemu virt platform.
>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
> OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index d3624e899e8d..6bc7c90f31dc 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,7 +203,7 @@ [PcdsFeatureFlag]
> gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>
> [PcdsFixedAtBuild.common]
> - gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFE
> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFC
> gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
> gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
> gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113175): https://edk2.groups.io/g/devel/message/113175
Mute This Topic: https://groups.io/mt/103501846/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension
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
1 sibling, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2024-01-04 14:38 UTC (permalink / raw)
To: devel, sunilvl
Cc: Andrei Warkentin, Ard Biesheuvel, Gerd Hoffmann, Jiewen Yao
On 1/3/24 14:58, Sunil V L wrote:
> Override Sstc extension and use SBI calls itself by default for RISC-V
> qemu virt platform.
>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
> OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index d3624e899e8d..6bc7c90f31dc 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,7 +203,7 @@ [PcdsFeatureFlag]
> gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>
> [PcdsFixedAtBuild.common]
> - gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFE
> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFC
> gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
> gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
> gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113177): https://edk2.groups.io/g/devel/message/113177
Mute This Topic: https://groups.io/mt/103501846/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension
2024-01-03 13:58 [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension Sunil V L
` (3 preceding siblings ...)
2024-01-03 13:58 ` [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension Sunil V L
@ 2024-01-05 19:10 ` Pedro Falcato
2024-01-08 4:06 ` Sunil V L
4 siblings, 1 reply; 14+ messages in thread
From: Pedro Falcato @ 2024-01-05 19:10 UTC (permalink / raw)
To: devel, sunilvl
Cc: Andrei Warkentin, Ard Biesheuvel, Gerd Hoffmann, Jiewen Yao,
Laszlo Ersek, Rahul Kumar, Ray Ni, Michael D Kinney, Liming Gao,
Zhiguang Liu
On Wed, Jan 3, 2024 at 1:59 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> This series adds the support for RISV-V Sstc extension in EDK2 timer
nit: RISC-V
> implementation. Sstc extension allows S-mode software to program the
> timer directly without using SBI calls.
>
> Currently, PCD variable is used to detect whether feature is enabled. By
> default the feature is enabled and platforms need to set the PCD to
> disable the feature if Sstc is not supported.
>
> For RiscVVirtQemu, it is disabled by default (until extension discovery
> feature is enabled).
I'm curious, what do you want Sstc for? Is the performance difference
measurable (if so, please post numbers, and add them to the commit)?
Does it have any other advantages?
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113315): https://edk2.groups.io/g/devel/message/113315
Mute This Topic: https://groups.io/mt/103501836/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension
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
0 siblings, 0 replies; 14+ messages in thread
From: Sunil V L @ 2024-01-08 4:06 UTC (permalink / raw)
To: Pedro Falcato
Cc: devel, Andrei Warkentin, Ard Biesheuvel, Gerd Hoffmann,
Jiewen Yao, Laszlo Ersek, Rahul Kumar, Ray Ni, Michael D Kinney,
Liming Gao, Zhiguang Liu
Hi Pedro,
On Fri, Jan 05, 2024 at 07:10:40PM +0000, Pedro Falcato wrote:
> On Wed, Jan 3, 2024 at 1:59 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > This series adds the support for RISV-V Sstc extension in EDK2 timer
>
> nit: RISC-V
> > implementation. Sstc extension allows S-mode software to program the
> > timer directly without using SBI calls.
> >
> > Currently, PCD variable is used to detect whether feature is enabled. By
> > default the feature is enabled and platforms need to set the PCD to
> > disable the feature if Sstc is not supported.
> >
> > For RiscVVirtQemu, it is disabled by default (until extension discovery
> > feature is enabled).
>
> I'm curious, what do you want Sstc for? Is the performance difference
> measurable (if so, please post numbers, and add them to the commit)?
> Does it have any other advantages?
>
Good question.
Without Sstc, timer needs to be programmed using SBI call. The number of
instructions via SBI call is way more than a simple CSR access. So, when
the CPU supports Sstc, there is no point in using SBI call. The issue
with SBI call will be worse under KVM since it has to emulate the timer.
Supporting Sstc is futuristic. The platforms can decide not to implement
SBI TIME interface at all since they have Sstc. In that case, EDK2 needs
to have option to support either.
Thanks,
Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113353): https://edk2.groups.io/g/devel/message/113353
Mute This Topic: https://groups.io/mt/103501836/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 14+ messages in thread