[RVV] CVA6 re-parametrization and MMU interface #2652

mp-17 · 2024-12-04T21:46:28Z

Follow-up to the discussion on extending Linux support to the Ara vector processor.

Main changes:

Addition:

Add external MMU interface to share the MMU with the external accelerator.

Modifications:

2 commit ports by default in cv64a6_imafdcv_config_pkg.
Change exception_t from localparam to param in cva6.sv.
Add parameters accelerator_req_t, accelerator_resp_t, acc_mmu_req_t, and acc_mmu_resp_t to cva6.sv.
Use spill register in acc_dispatcher to fully decouple timing.
Decrease cache sizes in cv64a6_imafdcv_sv39_config_pkg.

Collateral changes:

Fixes:

Guard some X-IF code lines with correct parameter in cva6.sv.
Parametrize the tracer interface with NrCommitPorts.
Fix parametrization for PMPEntries when NrPMPEntries == 0.
Add missing local dependencies to Bender.yml.
Guard some icache lines with FETCH_USER_EN.

Todo:

Clean with Verible
Test this rebased version with Ara

github-actions · 2024-12-04T21:47:21Z

core/acc_dispatcher.sv

+    parameter type acc_resp_t = logic,
+    parameter type accelerator_req_t = logic,
+    parameter type accelerator_resp_t = logic,
+    parameter type acc_mmu_req_t  = logic,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

parameter type acc_mmu_req_t = logic,

parameter type acc_mmu_req_t = logic,

github-actions · 2024-12-04T21:47:21Z

core/acc_dispatcher.sv

+  logic     acc_req_valid;
+  logic     acc_req_ready;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic acc_req_valid;

logic acc_req_ready;

logic acc_req_valid;

logic acc_req_ready;

github-actions · 2024-12-04T21:47:21Z

core/acc_dispatcher.sv

      .clk_i     (clk_i),
      .rst_ni    (rst_ni),
-      .clr_i     (1'b0),
-      .testmode_i(1'b0),
      .data_i    (acc_req),
      .valid_i   (acc_req_valid),
      .ready_o   (acc_req_ready),
      .data_o    (acc_req_int),
-      .valid_o   (acc_req_o.req_valid),
-      .ready_i   (acc_resp_i.req_ready)
+      .valid_o   (acc_req_o.acc_req.req_valid),
+      .ready_i   (acc_resp_i.acc_resp.req_ready)


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

.clk_i (clk_i),

.rst_ni (rst_ni),

.clr_i (1'b0),

.testmode_i(1'b0),

.data_i (acc_req),

.valid_i (acc_req_valid),

.ready_o (acc_req_ready),

.data_o (acc_req_int),

.valid_o (acc_req_o.req_valid),

.ready_i (acc_resp_i.req_ready)

.valid_o (acc_req_o.acc_req.req_valid),

.ready_i (acc_resp_i.acc_resp.req_ready)

.clk_i (clk_i),

.rst_ni (rst_ni),

.data_i (acc_req),

.valid_i(acc_req_valid),

.ready_o(acc_req_ready),

.data_o (acc_req_int),

.valid_o(acc_req_o.acc_req.req_valid),

.ready_i(acc_resp_i.acc_resp.req_ready)

github-actions · 2024-12-04T21:47:21Z

core/acc_dispatcher.sv

+  assign acc_req_o.acc_mmu_resp = acc_mmu_resp_i;
+  assign acc_req_o.acc_mmu_en   = acc_mmu_en_i;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

assign acc_req_o.acc_mmu_resp = acc_mmu_resp_i;

assign acc_req_o.acc_mmu_en = acc_mmu_en_i;

assign acc_req_o.acc_mmu_resp = acc_mmu_resp_i;

assign acc_req_o.acc_mmu_en = acc_mmu_en_i;

github-actions · 2024-12-04T21:47:21Z

core/acc_dispatcher.sv

+  assign acc_trans_id_o     = acc_resp_i.acc_resp.trans_id;
+  assign acc_result_o       = acc_resp_i.acc_resp.result;
+  assign acc_valid_o        = acc_resp_i.acc_resp.resp_valid;
+  assign acc_exception_o    = acc_resp_i.acc_resp.exception;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

assign acc_trans_id_o = acc_resp_i.acc_resp.trans_id;

assign acc_result_o = acc_resp_i.acc_resp.result;

assign acc_valid_o = acc_resp_i.acc_resp.resp_valid;

assign acc_exception_o = acc_resp_i.acc_resp.exception;

assign acc_trans_id_o = acc_resp_i.acc_resp.trans_id;

assign acc_result_o = acc_resp_i.acc_resp.result;

assign acc_valid_o = acc_resp_i.acc_resp.resp_valid;

assign acc_exception_o = acc_resp_i.acc_resp.exception;

github-actions · 2024-12-04T21:47:23Z

core/load_store_unit.sv

      end
    end
  end

+  if (CVA6Cfg.EnableAccelerator) begin
+    // The MMU can be connected to CVA6 or the ACCELERATOR
+    enum logic {CVA6, ACC} mmu_state_d, mmu_state_q;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

enum logic {CVA6, ACC} mmu_state_d, mmu_state_q;

enum logic {

CVA6,

ACC

}

mmu_state_d, mmu_state_q;

github-actions · 2024-12-04T21:47:23Z

core/load_store_unit.sv

+    // This logic can be optimized to reduce answer latency and contention
+    always_comb begin
+      // Maintain state
+      mmu_state_d = mmu_state_q;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

mmu_state_d = mmu_state_q;

mmu_state_d = mmu_state_q;

github-actions · 2024-12-04T21:47:23Z

core/load_store_unit.sv

+    assign cva6_dtlb_hit          = dtlb_hit;
+    assign cva6_dtlb_ppn          = dtlb_ppn;
+    // No accelerator
+    assign acc_mmu_resp_o = '0;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

assign acc_mmu_resp_o = '0;

assign acc_mmu_resp_o = '0;

github-actions · 2024-12-04T21:47:23Z

core/load_store_unit.sv

+    // No accelerator
+    assign acc_mmu_resp_o = '0;
+    // Feed forward the lsu_ctrl bypass
+    assign lsu_ctrl = lsu_ctrl_byp;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

assign lsu_ctrl = lsu_ctrl_byp;

assign lsu_ctrl = lsu_ctrl_byp;

github-actions · 2024-12-04T21:47:23Z

core/load_store_unit.sv

    ld_valid_i        = 1'b0;
    st_valid_i        = 1'b0;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

ld_valid_i = 1'b0;

st_valid_i = 1'b0;

ld_valid_i = 1'b0;

st_valid_i = 1'b0;

github-actions · 2024-12-04T21:54:06Z

❌ failed run, report available here.

JeanRochCoulon · 2024-12-05T08:08:48Z

Hello @mp-17 To solve the Verible format issues, you can execute the Verible command from https://github.com/openhwgroup/cva6/blob/master/CONTRIBUTING.md

mp-17 · 2024-12-05T08:37:53Z

Hello @JeanRochCoulon, thanks, I will! The PR is still work in progress, I put it here as a placeholder. I will rework it today and ping you when it's ready. Don't spend time on the diff now, it's full of unrelated stuff :-)

…void warnings

github-actions · 2024-12-05T10:33:02Z

core/acc_dispatcher.sv

  assign acc_ld_disp          = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_LOAD);
  assign acc_st_disp          = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_STORE);


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

assign acc_ld_disp = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_LOAD);

assign acc_st_disp = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_STORE);

assign acc_ld_disp = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_LOAD);

assign acc_st_disp = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_STORE);

github-actions · 2024-12-05T10:33:02Z

core/load_store_unit.sv

+  logic       [                     31:0] mmu_tinst;
+  logic                                   mmu_hs_ld_st_inst;
+  logic                                   mmu_hlvx_inst;
+  exception_t                             mmu_exception, cva6_mmu_exception, acc_mmu_exception;
+  exception_t                             pmp_exception;
+  icache_areq_t                           pmp_icache_areq_i;
+  logic                                   pmp_translation_valid;
+  logic                                   dtlb_hit, cva6_dtlb_hit, acc_dtlb_hit;
+  logic       [         CVA6Cfg.PPNW-1:0] dtlb_ppn, cva6_dtlb_ppn, acc_dtlb_ppn;
+
+  logic                                   ld_valid;
+  logic       [CVA6Cfg.TRANS_ID_BITS-1:0] ld_trans_id;
+  logic       [         CVA6Cfg.XLEN-1:0] ld_result;
+  logic                                   st_valid;
+  logic       [CVA6Cfg.TRANS_ID_BITS-1:0] st_trans_id;
+  logic       [         CVA6Cfg.XLEN-1:0] st_result;
+
+  logic       [                     11:0] page_offset;
+  logic                                   page_offset_matches;
+
+  exception_t                             misaligned_exception, cva6_misaligned_exception, acc_misaligned_exception;
+  exception_t                             ld_ex;
+  exception_t                             st_ex;
+
+  logic                                   hs_ld_st_inst;
+  logic                                   hlvx_inst;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic [ 31:0] mmu_tinst;

logic mmu_hs_ld_st_inst;

logic mmu_hlvx_inst;

exception_t mmu_exception, cva6_mmu_exception, acc_mmu_exception;

exception_t pmp_exception;

icache_areq_t pmp_icache_areq_i;

logic pmp_translation_valid;

logic dtlb_hit, cva6_dtlb_hit, acc_dtlb_hit;

logic [ CVA6Cfg.PPNW-1:0] dtlb_ppn, cva6_dtlb_ppn, acc_dtlb_ppn;

logic ld_valid;

logic [CVA6Cfg.TRANS_ID_BITS-1:0] ld_trans_id;

logic [ CVA6Cfg.XLEN-1:0] ld_result;

logic st_valid;

logic [CVA6Cfg.TRANS_ID_BITS-1:0] st_trans_id;

logic [ CVA6Cfg.XLEN-1:0] st_result;

logic [ 11:0] page_offset;

logic page_offset_matches;

exception_t misaligned_exception, cva6_misaligned_exception, acc_misaligned_exception;

exception_t ld_ex;

exception_t st_ex;

logic hs_ld_st_inst;

logic hlvx_inst;

logic [31:0] mmu_tinst;

logic mmu_hs_ld_st_inst;

logic mmu_hlvx_inst;

exception_t mmu_exception, cva6_mmu_exception, acc_mmu_exception;

exception_t pmp_exception;

icache_areq_t pmp_icache_areq_i;

logic pmp_translation_valid;

logic dtlb_hit, cva6_dtlb_hit, acc_dtlb_hit;

logic [CVA6Cfg.PPNW-1:0] dtlb_ppn, cva6_dtlb_ppn, acc_dtlb_ppn;

logic ld_valid;

logic [CVA6Cfg.TRANS_ID_BITS-1:0] ld_trans_id;

logic [ CVA6Cfg.XLEN-1:0] ld_result;

logic st_valid;

logic [CVA6Cfg.TRANS_ID_BITS-1:0] st_trans_id;

logic [ CVA6Cfg.XLEN-1:0] st_result;

logic [ 11:0] page_offset;

logic page_offset_matches;

exception_t misaligned_exception, cva6_misaligned_exception, acc_misaligned_exception;

exception_t ld_ex;

exception_t st_ex;

logic hs_ld_st_inst;

logic hlvx_inst;

github-actions · 2024-12-05T10:37:10Z

❌ failed run, report available here.

github-actions · 2024-12-16T13:05:41Z

core/cache_subsystem/wt_dcache_missunit.sv

+  function automatic logic [CVA6Cfg.PLEN-1:0] paddrSizeAlign(
+      input logic [CVA6Cfg.PLEN-1:0] paddr, input logic [2:0] size);


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

function automatic logic [CVA6Cfg.PLEN-1:0] paddrSizeAlign(

input logic [CVA6Cfg.PLEN-1:0] paddr, input logic [2:0] size);

function automatic logic [CVA6Cfg.PLEN-1:0] paddrSizeAlign(input logic [CVA6Cfg.PLEN-1:0] paddr,

input logic [2:0] size);

github-actions · 2024-12-16T13:05:42Z

core/cache_subsystem/wt_dcache_wbuffer.sv

+      input logic [CVA6Cfg.XLEN-1:0] data,
+      input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset, input logic [1:0] size);


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

input logic [CVA6Cfg.XLEN-1:0] data,

input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset, input logic [1:0] size);

input logic [CVA6Cfg.XLEN-1:0] data, input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset,

input logic [1:0] size);

github-actions · 2024-12-16T13:05:42Z

core/cache_subsystem/wt_dcache_wbuffer.sv

+      input logic [CVA6Cfg.XLEN-1:0] data,
+      input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset, input logic [1:0] size);


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

input logic [CVA6Cfg.XLEN-1:0] data,

input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset, input logic [1:0] size);

input logic [CVA6Cfg.XLEN-1:0] data, input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset,

input logic [1:0] size);

github-actions · 2024-12-16T13:05:42Z

core/cva6_mmu/cva6_shared_tlb.sv

+  logic [CVA6Cfg.SharedTlbDepth-1:0][SHARED_TLB_WAYS-1:0]
+      shared_tag_valid_q, shared_tag_valid_d;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic [CVA6Cfg.SharedTlbDepth-1:0][SHARED_TLB_WAYS-1:0]

shared_tag_valid_q, shared_tag_valid_d;

logic [CVA6Cfg.SharedTlbDepth-1:0][SHARED_TLB_WAYS-1:0] shared_tag_valid_q, shared_tag_valid_d;

github-actions · 2024-12-16T13:05:42Z

core/cva6_mmu/cva6_shared_tlb.sv

+  logic [CVA6Cfg.PtLevels+HYP_EXT-1:0][(CVA6Cfg.VpnLen/CVA6Cfg.PtLevels)-1:0]
+      vpn_d, vpn_q;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic [CVA6Cfg.PtLevels+HYP_EXT-1:0][(CVA6Cfg.VpnLen/CVA6Cfg.PtLevels)-1:0]

vpn_d, vpn_q;

logic [CVA6Cfg.PtLevels+HYP_EXT-1:0][(CVA6Cfg.VpnLen/CVA6Cfg.PtLevels)-1:0] vpn_d, vpn_q;

github-actions · 2024-12-16T13:05:42Z

core/frontend/frontend.sv

+  logic [CVA6Cfg.INSTR_PER_FETCH-1:0]
+      rvc_branch, rvc_jump, rvc_jr, rvc_return, rvc_jalr, rvc_call;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic [CVA6Cfg.INSTR_PER_FETCH-1:0]

rvc_branch, rvc_jump, rvc_jr, rvc_return, rvc_jalr, rvc_call;

logic [CVA6Cfg.INSTR_PER_FETCH-1:0] rvc_branch, rvc_jump, rvc_jr, rvc_return, rvc_jalr, rvc_call;

github-actions · 2024-12-16T13:05:42Z

core/pmp/tb/tb_pkg.sv

+    static function logic [PMP_LEN-1:0] base_to_conf(logic [WIDTH-1:0] base,
+                                                       int unsigned size_i);


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

static function logic [PMP_LEN-1:0] base_to_conf(logic [WIDTH-1:0] base,

int unsigned size_i);

static function logic [PMP_LEN-1:0] base_to_conf(logic [WIDTH-1:0] base, int unsigned size_i);

github-actions · 2024-12-16T13:05:43Z

core/scoreboard.sv

+  logic [CVA6Cfg.NrCommitPorts-1:0][CVA6Cfg.TRANS_ID_BITS-1:0]
+      commit_pointer_n, commit_pointer_q;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic [CVA6Cfg.NrCommitPorts-1:0][CVA6Cfg.TRANS_ID_BITS-1:0]

commit_pointer_n, commit_pointer_q;

logic [CVA6Cfg.NrCommitPorts-1:0][CVA6Cfg.TRANS_ID_BITS-1:0] commit_pointer_n, commit_pointer_q;

mp-17 · 2024-12-16T13:08:30Z

Hi @JeanRochCoulon, I cleaned up the code diff so that we can start discussing how to integrate the interface and re-parametrization modifications.
Let me know what you think of this PR or if you need additional refactoring or explanations before reviewing.

I have added additional localparams (such as PLEN) to the cv64a6_imafdcv_sv39_config_pkg.sv user config package. Before adapting the other packages too (to fix the CI), let me know if this way of handling the issue is okay or if we need to refine it.

github-actions · 2024-12-16T16:44:14Z

❌ failed run, report available here.

JeanRochCoulon · 2024-12-18T05:48:10Z

@cathales You should have suggestions to provide. @mp-17 tries to declare parameters needed for CV-X-IF/ARA integration.

cathales

It seems that you add fields to cva6_user_cfg_t not because you need to assign specific values to them, but because you want to access them in your configuration package.
This will impact all configurations and imply repetition.

Instead, you could call build_config once to be able to have access to the calculated values.

localparam cfg = build_config(cva6_cfg);

However, this would prevent from creating heterogeneous designs.

If you have one top-level module above cva6, I advise you to not add types into cv64a6_imafdcv_sv39_config_pkg. Instead, you can add them as localparam right into your top-level module and propagate them to cva6 and other modules.
To access to all calculated values, first add at the beginning of your top-level module:

    parameter config_pkg::cva6_cfg_t CVA6Cfg = build_config_pkg::build_config(
        cva6_config_pkg::cva6_cfg
    ),

This is how it is done in cva6.sv.

Also, when you instantiate CVA6, you can add cva6 #(.CVA6Cfg(CVA6Cfg)) parameter to re-use the configuration that you already calculated instead of implicitly calling build_config again to build the default value. This is what allows heterogeneous designs.

If you need several top level modules, we have another solution that we use for RVFI, just ask me if you need it.

PS: These modifications seem related to accelerator port, what about using CV-X-IF instead?

mp-17 · 2024-12-18T09:26:21Z

Hello @cathales,

Thanks for the reply!

I think you interpreted it correctly. I want to have a single source of truth for data types (e.g., exception_t, interfaces) and parameters so that I can reuse them both in CVA6 and in the tightly-coupled accelerator.
From my understanding, you would prefer all these parameters to be defined in my top level only and then assigned as parameters to CVA6 and the accelerator.
I will clean up the cv64a6_imafdcv_sv39_config_pkg.sv and try as you suggested. I will tell you if I meet any blockers.
CV-X-IF: The plan discussed during our last joint meeting is to merge these custom interface modifications first and then discuss how to transition to the CV-X-IF later. This is also because CV-X-IF would need a custom memory interface anyway.

github-actions · 2024-12-19T17:22:22Z

core/cva6_mmu/cva6_mmu.sv

@@ -100,8 +100,8 @@ module cva6_mmu

    // PMP

-    input riscv::pmpcfg_t [CVA6Cfg.NrPMPEntries-1:0]                   pmpcfg_i,
-    input logic           [CVA6Cfg.NrPMPEntries-1:0][CVA6Cfg.PLEN-3:0] pmpaddr_i
+    input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0]                   pmpcfg_i,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0] pmpcfg_i,

input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0] pmpcfg_i,

github-actions · 2024-12-19T17:22:23Z

core/load_store_unit.sv

@@ -148,9 +154,9 @@ module load_store_unit
    input  amo_resp_t           amo_resp_i,

    // PMP configuration - CSR_REGFILE
-    input riscv::pmpcfg_t [CVA6Cfg.NrPMPEntries-1:0]                   pmpcfg_i,
+    input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0]                   pmpcfg_i,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0] pmpcfg_i,

input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0] pmpcfg_i,

mp-17 · 2024-12-19T17:32:02Z

Hi @cathales,

I have implemented the requested modifications.

As now listed in the PR description, I needed to make further collateral modifications and fixes to compile the code with my settings.

Let me know if you prefer I split this PR into a main and a collateral PR.

Also, please let me know if the interfaces are now okay or if I need to implement other modifications.

github-actions · 2024-12-19T18:11:03Z

✔️ successful run, report available here.

github-actions · 2024-12-19T18:17:26Z

✔️ successful run, report available here.

cathales

LGTM for the interfaces and parametrization.

@yanicasa can you please review the changes in core/load_store_unit.sv?

cathales · 2024-12-20T08:57:48Z

core/pmp/src/pmp.sv

-    input logic [NR_ENTRIES-1:0][PMP_LEN-1:0] conf_addr_i,
-    input riscv::pmpcfg_t [NR_ENTRIES-1:0] conf_i,
+    input logic [(NR_ENTRIES > 0 ? NR_ENTRIES-1 : 0):0][PMP_LEN-1:0] conf_addr_i,
+    input riscv::pmpcfg_t [(NR_ENTRIES > 0 ? NR_ENTRIES-1 : 0):0] conf_i,


You repeat this, or equivalently CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0, many times.
You could add this to the configuration built with build_config to be more DRY

Sure! I originally stuck to what I found in the csr_regfile module not to introduce additional style modifications. I will change that, too.

Actually I am not 100% sold on the definition of an abstract quantity in the build_config.
Maybe a avoid_neg() function is cleaner?
Something like:

Pseudocode

// Defined in a package function avoid_neg(a) return (a < 0) ? 0 : a; ... // Interface signal declaration input logic [avoid_neg(NR_ENTRIES-1):0][PMP_LEN-1:0] conf_addr_i

Would this work?

I think so, and it could be used at many places!

I’m wondering if a function which would do x -> avoid_neg(x - 1) would be good as it seems to be always the same use-case… But I cannot find a clear name for it; and I think writing/reading avoid_neg(foo - 1) already makes the intent really clear.

True! I will think about it as well, even if it's already better than before.
In the meantime, I pushed the avoid_neg button to clean the diff a bit.

Another argument for avoid_neg: we are already used to write arrays shape as [Length-1:0] and [avoid_neg(Length-1):0] is closer to this than than [foo(Length):0] is.

cathales · 2024-12-20T09:13:04Z

core/cva6.sv

+    // Accelerator - CVA6
+    parameter type accelerator_req_t  = logic,
+    parameter type accelerator_resp_t = logic,
+
+    // Accelerator - CVA6's MMU
+    parameter type acc_mmu_req_t  = logic,
+    parameter type acc_mmu_resp_t = logic,


We will need to document somewhere what is expected in these structures, and the protocol used.

But I think it can wait, especially if we will soon switch to CV-X-IF.

cathales · 2024-12-20T09:25:53Z

core/cva6.sv

+    // branchpredict scoreboard entry
+    // this is the struct which we will inject into the pipeline to guide the various
+    // units towards the correct branch decision and resolve
+    localparam type branchpredict_sbe_t = struct packed {
+      cf_t                     cf;               // type of control flow prediction
+      logic [CVA6Cfg.VLEN-1:0] predict_address;  // target address at which to jump, or not
+    },


Can you please restore the previous order to reduce the diff? Unless this change is needed, of course.

We already have parameter later in the configuration so we do not have to keep parameter above localparam here.

github-actions · 2024-12-20T13:50:57Z

❌ failed run, report available here.

github-actions · 2024-12-20T15:06:27Z

❌ failed run, report available here.

github-actions · 2024-12-20T15:48:49Z

❌ failed run, report available here.

github-actions · 2024-12-20T15:54:47Z

✔️ successful run, report available here.

github-actions bot reviewed Dec 4, 2024

View reviewed changes

mp-17 added 12 commits December 5, 2024 10:53

acc_dispatcher, load_store_unit: add mmu interface for accelerator

e8f721b

load_store_unit: add MMU accelerator port

0817830

ara: fix parametrization

9cfd276

tracer: parametrize interface on NrCommitPorts

8cf71b1

cva6: 🐛 guard xif signals within CvxifEn

be4f21f

acc_dispatcher: add accelerator_req interface definitions

c992be3

acc_dispatcher: fix signal names

3752dce

icache: guard user-icache-line assignments with FETCH_USER_EN

8ca94e1

cv64a6_imafdcv_sv39_config_pkg: use safe value of unused feature to a…

bf13267

…void warnings

acc_dispatcher: cut combinatorial path from CVA6 to Ara

cd91e72

cv64a6_imafdcv_sv39: 2 commit ports by default

477c1c2

core: re-parametrize the RVV interfaces

ab64842

mp-17 force-pushed the mp/pulp-v2 branch from f695016 to ab64842 Compare December 5, 2024 10:32

github-actions bot reviewed Dec 5, 2024

View reviewed changes

treewide: verible pass

d960b71

github-actions bot reviewed Dec 16, 2024

View reviewed changes

cathales reviewed Dec 18, 2024

View reviewed changes

github-actions bot reviewed Dec 19, 2024

View reviewed changes

mp-17 force-pushed the mp/pulp-v2 branch from 3dc6cdc to 61625e2 Compare December 19, 2024 17:22

cathales reviewed Dec 20, 2024

View reviewed changes

mp-17 added 3 commits December 20, 2024 14:42

Fix PMPEntries parametrization when PMPEntries == 0

35e5850

config_pkg: clean-up config packages

01b26d5

bender: fix missing local file dependencies

b26479b

mp-17 force-pushed the mp/pulp-v2 branch from 61625e2 to b26479b Compare December 20, 2024 13:43

mp-17 added 3 commits December 20, 2024 15:19

cv64a6_imafdcv_sv39_config_pkg: decrease CVA6's cache size

70b0f90

cva6: swap parameters to original order

40ba10c

Introduce avoid_neg() guard function for array sizing

9104f41

mp-17 force-pushed the mp/pulp-v2 branch from e000e6a to 9104f41 Compare December 20, 2024 15:02

	parameter type acc_mmu_req_t = logic,
	parameter type acc_mmu_req_t = logic,

		assign acc_req_o.acc_mmu_resp = acc_mmu_resp_i;
		assign acc_req_o.acc_mmu_en = acc_mmu_en_i;

	assign lsu_ctrl = lsu_ctrl_byp;
	assign lsu_ctrl = lsu_ctrl_byp;

		assign acc_ld_disp = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_LOAD);
		assign acc_st_disp = acc_req_valid && (acc_insn_queue_o.operation == ACCEL_OP_STORE);

		function automatic logic [CVA6Cfg.PLEN-1:0] paddrSizeAlign(
		input logic [CVA6Cfg.PLEN-1:0] paddr, input logic [2:0] size);

		input logic [CVA6Cfg.XLEN-1:0] data,
		input logic [CVA6Cfg.XLEN_ALIGN_BYTES-1:0] offset, input logic [1:0] size);

		logic [CVA6Cfg.SharedTlbDepth-1:0][SHARED_TLB_WAYS-1:0]
		shared_tag_valid_q, shared_tag_valid_d;

		logic [CVA6Cfg.PtLevels+HYP_EXT-1:0][(CVA6Cfg.VpnLen/CVA6Cfg.PtLevels)-1:0]
		vpn_d, vpn_q;

		logic [CVA6Cfg.INSTR_PER_FETCH-1:0]
		rvc_branch, rvc_jump, rvc_jr, rvc_return, rvc_jalr, rvc_call;

	logic [CVA6Cfg.INSTR_PER_FETCH-1:0]
	rvc_branch, rvc_jump, rvc_jr, rvc_return, rvc_jalr, rvc_call;
	logic [CVA6Cfg.INSTR_PER_FETCH-1:0] rvc_branch, rvc_jump, rvc_jr, rvc_return, rvc_jalr, rvc_call;

		static function logic [PMP_LEN-1:0] base_to_conf(logic [WIDTH-1:0] base,
		int unsigned size_i);

		logic [CVA6Cfg.NrCommitPorts-1:0][CVA6Cfg.TRANS_ID_BITS-1:0]
		commit_pointer_n, commit_pointer_q;

	input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0] pmpcfg_i,
	input riscv::pmpcfg_t [(CVA6Cfg.NrPMPEntries > 0 ? CVA6Cfg.NrPMPEntries-1 : 0):0] pmpcfg_i,

[RVV] CVA6 re-parametrization and MMU interface #2652

Are you sure you want to change the base?

[RVV] CVA6 re-parametrization and MMU interface #2652

Conversation

mp-17 commented Dec 4, 2024 • edited Loading

Main changes:

Collateral changes:

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024

JeanRochCoulon commented Dec 5, 2024

mp-17 commented Dec 5, 2024

github-actions bot Dec 5, 2024

Choose a reason for hiding this comment

github-actions bot Dec 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2024

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot Dec 16, 2024

Choose a reason for hiding this comment

mp-17 commented Dec 16, 2024 • edited Loading

github-actions bot commented Dec 16, 2024

JeanRochCoulon commented Dec 18, 2024

cathales left a comment

Choose a reason for hiding this comment

mp-17 commented Dec 18, 2024

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

mp-17 commented Dec 19, 2024 • edited Loading

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

cathales left a comment

Choose a reason for hiding this comment

cathales Dec 20, 2024

Choose a reason for hiding this comment

mp-17 Dec 20, 2024

Choose a reason for hiding this comment

mp-17 Dec 20, 2024

Choose a reason for hiding this comment

cathales Dec 20, 2024

Choose a reason for hiding this comment

mp-17 Dec 20, 2024

Choose a reason for hiding this comment

cathales Dec 20, 2024

Choose a reason for hiding this comment

cathales Dec 20, 2024

Choose a reason for hiding this comment

mp-17 commented Dec 4, 2024 •

edited

Loading

mp-17 commented Dec 16, 2024 •

edited

Loading

mp-17 commented Dec 19, 2024 •

edited

Loading

cathales Dec 20, 2024 •

edited

Loading