Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the default number of performance counters from 0 to 10 and replace all instances of Ibex with cve2. #279

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cairo-caplan
Copy link

… (#214)

rtl/cve2_cs_registers.sv did not need to be changed as its default value was already 10

@MikeOpenHWGroup
Copy link
Member

LGTM @cairo-caplan. Since we are editing rtl/cve2_top.sv and rtl/cve2_core.sv already, can you replace "ibex" with "cve2" in the top-level comment?

@MikeOpenHWGroup MikeOpenHWGroup changed the title [rtl] Changed the default number of performance counters from 0 to 10… Change the default number of performance counters from 0 to 10 and replace all instances of Ibex with cve2. Feb 18, 2025
Copy link
Member

@MikeOpenHWGroup MikeOpenHWGroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that was a lot more than I was expecting. I have taken the liberty of updating the PR title to reflect that almost all instances of Ibex have been replaced with cve2.

README.md Outdated
@@ -74,7 +74,7 @@ Notes:
Red indicates a configuration with minimal/no verification.
* v.1.0.0 of the RISC-V Bit-Manipulation Extension is supported as well as the remaining sub-extensions of draft v.0.93 of the bitmanip spec.
The latter is *not ratified* and there may be changes before ratification.
See [Standards Compliance](https://ibex-core.readthedocs.io/en/latest/01_overview/compliance.html) in the Ibex documentation for more information.
See [Standards Compliance](https://cve2-core.readthedocs.io/en/latest/01_overview/compliance.html) in the CVE2 documentation for more information.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link has two issues:

  1. It should be of the form https://docs.openhwgroup.org/projects/cve2-user-manual/en/latest/.
  2. The compliance section does not seem to exist...

@@ -28,4 +28,4 @@ resources:
name: lowrisc/lowrisc-private-ci

extends:
template: jobs-ibex.yml@lowrisc-private-ci
template: jobs-cve2.yml@lowrisc-private-ci
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt this will work since lowrisc-private-ci will not have a template for jobs-cve2.

ci/vars.yml Outdated
# lowRISC-internal version numbers of Ibex-specific Spike builds.
SPIKE_IBEX_VERSION: "20211027-git-ec461195803d0a8a72c90d52dbb38ad24ac7c55d"
# lowRISC-internal version numbers of CVE2-specific Spike builds.
SPIKE_CVE2_VERSION: "20211027-git-ec461195803d0a8a72c90d52dbb38ad24ac7c55d"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work? I think so, but am not sure.

@@ -36,7 +36,7 @@ or as the processor element in embedded :term:`SoC` subsystems.

The core design was originally developed as the PULPino *ZeroRISCY*
processor by ETH Zürich and the University of Bologna and later enhanced
by the lowRISC consortium as the *Ibex* core. For this project, the Ibex
by the lowRISC consortium as the *CVE2* core. For this project, the CVE2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be edited a bit to be correct. Maybe something like this:

The core design was originally developed as the PULPino *ZeroRISCY*
processor by ETH Zürich and the University of Bologna and later enhanced
by the lowRISC consortium as the *Ibex* core.  In 2023 a fork of Ibex was made
by the OpenHW Group and given the name *CVE2*.  The CVE2 is a simplified version
of Ibex and qualified using the industrial-strength
Core-V-Verification methodologies. The source :term:`RTL` code is written in

Much of the code was developed by simplifying the RV32 CPU core "RI5CY" to demonstrate how small a RISC-V CPU core could actually be `[1] <https://doi.org/10.1109/PATMOS.2017.8106976>`_.
To make it even smaller, support for the "E" extension was added under the code name "Micro-riscy".
In the PULP ecosystem, the core is used as the control core for PULP, PULPino and PULPissimo.

In December 2018 lowRISC took over the development of Zero-riscy and renamed it to Ibex.
In December 2018 lowRISC took over the development of Zero-riscy and renamed it to CVE2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that statement was correct as-is. Suggest something like this:

In December 2018 lowRISC took over the development of Zero-riscy and renamed it to Ibex.
In 2023 the OpenHW Group forked Ibex and renamed the fork cve2.

@@ -20,10 +20,10 @@ Testbench Architecture
.. figure:: images/tb2.svg
:alt: Testbench Architecture

Architecture of the UVM testbench for Ibex core
Architecture of the UVM testbench for CVE2 core
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not using this verification environment. I have created task #281 to resolve.

doc/Makefile Outdated
@@ -4,7 +4,7 @@
# You can set these variables from the command line.
SPHINXOPTS =
SPHINXBUILD = sphinx-build
SPHINXPROJ = ibex
SPHINXPROJ = cve2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That might break the documentation build. We can accept the risk for now.

doc/index.rst Outdated
@@ -2,21 +2,21 @@ CV32E20: An embedded 32-bit RISC-V CPU core
===========================================

CV32E20 is a production-quality open source 32-bit RISC-V CPU core written in SystemVerilog.
The CPU core is based on the Ibex core, but simplified and verified under the OpenHW Group.
The CPU core is based on the CVE2 core, but simplified and verified under the OpenHW Group.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is still true that the CV32E20 is based on Ibex, so we should leave this one as is. If you like, you could update it to say this:

CV32E20 is a specific configuration of the CVE2 and is a production-quality open source 32-bit RISC-V CPU core written in SystemVerilog.
The CVE2 is based on the lowRISC Ibex core, but simplified and (re) verified by the OpenHW Group.

doc/make.bat Outdated
@@ -9,7 +9,7 @@ if "%SPHINXBUILD%" == "" (
)
set SOURCEDIR=.
set BUILDDIR=_build
set SPHINXPROJ=ibex
set SPHINXPROJ=cve2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment (might break the documentation build).

@@ -24,7 +24,7 @@ module cve2_riscv_compliance (
parameter cve2_pkg::regfile_e RegFile = cve2_pkg::RegFileFF;
parameter bit ICache = 1'b0;
parameter bit ICacheECC = 1'b0;
parameter bit SecureIbex = 1'b0;
parameter bit SecureCVE2 = 1'b0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this might negate my earlier comments...

Copy link
Member

@MikeOpenHWGroup MikeOpenHWGroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I requested a number of changes, so I need to withdraw my Approval.

@cairo-caplan cairo-caplan marked this pull request as draft February 19, 2025 09:42
@cairo-caplan cairo-caplan force-pushed the main branch 2 times, most recently from 16cd216 to a86d1b2 Compare February 28, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants