[edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension

[edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension

[Sunil V L]

This series adds the support for RISV-V Sstc extension in EDK2 timer
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).

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>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>

Sunil V L (4):
  MdePkg.dec: RISC-V: Define override bit for Sstc extension
  MdePkg/BaseLib: RISC-V: Add function to update stimecmp register
  UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc
  OvmfPkg/RiscVVirt: Override Sstc extension

 MdePkg/MdePkg.dec                             |  2 ++
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc           |  2 +-
 .../CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf |  1 +
 MdePkg/Include/Library/BaseLib.h              |  5 ++++
 .../Include/Register/RiscV64/RiscVEncoding.h  |  3 ++
 UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h         |  2 ++
 UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c         | 30 +++++++++++++++++--
 MdePkg/Library/BaseLib/RiscV64/ReadTimer.S    |  7 +++++
 8 files changed, 49 insertions(+), 3 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113092): https://edk2.groups.io/g/devel/message/113092
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/4] MdePkg.dec: RISC-V: Define override bit for Sstc extension

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/4] MdePkg/BaseLib: RISC-V: Add function to update stimecmp register

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/4] OvmfPkg/RiscVVirt: Override Sstc extension

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension

[Pedro Falcato]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension

[Sunil V L]

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]
-=-=-=-=-=-=-=-=-=-=-=-