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

Adding support for Scalar Crypto Extension (Bitmanip instructions for Cryptography, Zbkb) #2653

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

munailwaqar
Copy link

Introduction

This PR adds support for Zbkb extension in the CVA6 core. It also adds the documentation for this extension. These changes have been tested with self-written single instruction tests and with the riscv-arch-tests. This PR will be followed by other PRs that will add complete support for the Zkn - NIST Algorithm Suite extension.

Implementation

Zbkb Extension:
Added support for the Zbkb instruction set. It essentially expands the Zbb extension with additional instructions useful in cryptography. These instructions are pack, packh, packw, brev8, unzip and zip.

Modifications

  1. A new bit ZKN was added. The complete Zkn extension will be added under this bit for ease of use. This configuration will also require the RVB (bitmanip) bit to be set.
  2. Updated the ALU and decoder to recognize and handle Zbkb instructions.

Documentation and Reference

The official RISC-V Cryptography Extensions Volume I was followed to ensure alignment with ratification. The relevant documentation for the Zbkb instruction was also added.

Verification

Assembly Tests:
The instructions were tested and verified with the K module of both 32 bit and 64 bit versions of the riscv-arch-tests to ensure proper functionality. These tests check for ISA compliance, edge cases and use assertions to ensure expected behavior. The tests include:

  1. pack-01.S
  2. packh-01.S
  3. packw-01.S
  4. brev8-01.S
  5. unzip-01.S
  6. zip-01.S

core/alu.sv Outdated
Comment on lines 390 to 391
PACK: result_o = (CVA6Cfg.IS_XLEN32) ? ({fu_data_i.operand_b[15:0], fu_data_i.operand_a[15:0]}) : ({fu_data_i.operand_b[31:0], fu_data_i.operand_a[31:0]});
PACK_H: result_o = (CVA6Cfg.IS_XLEN32) ? ({16'b0, fu_data_i.operand_b[7:0], fu_data_i.operand_a[7:0]}) : ({48'b0, fu_data_i.operand_b[7:0], fu_data_i.operand_a[7:0]});
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
PACK: result_o = (CVA6Cfg.IS_XLEN32) ? ({fu_data_i.operand_b[15:0], fu_data_i.operand_a[15:0]}) : ({fu_data_i.operand_b[31:0], fu_data_i.operand_a[31:0]});
PACK_H: result_o = (CVA6Cfg.IS_XLEN32) ? ({16'b0, fu_data_i.operand_b[7:0], fu_data_i.operand_a[7:0]}) : ({48'b0, fu_data_i.operand_b[7:0], fu_data_i.operand_a[7:0]});
PACK:
result_o = (CVA6Cfg.IS_XLEN32) ? ({fu_data_i.operand_b[15:0], fu_data_i.operand_a[15:0]}) : ({fu_data_i.operand_b[31:0], fu_data_i.operand_a[31:0]});
PACK_H:
result_o = (CVA6Cfg.IS_XLEN32) ? ({16'b0, fu_data_i.operand_b[7:0], fu_data_i.operand_a[7:0]}) : ({48'b0, fu_data_i.operand_b[7:0], fu_data_i.operand_a[7:0]});

core/alu.sv Outdated
BREV8: result_o = brev8_reversed;
default: ;
endcase
if (fu_data_i.operation == PACK_W && CVA6Cfg.IS_XLEN64) result_o = {{32{fu_data_i.operand_b[15]}}, {fu_data_i.operand_b[15:0]}, {fu_data_i.operand_a[15:0]}};
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
if (fu_data_i.operation == PACK_W && CVA6Cfg.IS_XLEN64) result_o = {{32{fu_data_i.operand_b[15]}}, {fu_data_i.operand_b[15:0]}, {fu_data_i.operand_a[15:0]}};
if (fu_data_i.operation == PACK_W && CVA6Cfg.IS_XLEN64)
result_o = {
{32{fu_data_i.operand_b[15]}}, {fu_data_i.operand_b[15:0]}, {fu_data_i.operand_a[15:0]}
};

core/decoder.sv Outdated
Comment on lines 780 to 781
{7'b000_0100, 3'b100} : if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK; else illegal_instr_bm = 1'b1; //pack
{7'b000_0100, 3'b111} : if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_H; else illegal_instr_bm = 1'b1; //packh
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
{7'b000_0100, 3'b100} : if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK; else illegal_instr_bm = 1'b1; //pack
{7'b000_0100, 3'b111} : if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_H; else illegal_instr_bm = 1'b1; //packh
{
7'b000_0100, 3'b100
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK;
else illegal_instr_bm = 1'b1; //pack
{
7'b000_0100, 3'b111
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_H;
else illegal_instr_bm = 1'b1; //packh

core/decoder.sv Outdated
Comment on lines 855 to 856
{7'b011_0000, 3'b001}: instruction_o.op = ariane_pkg::ROLW; // rolw
{7'b011_0000, 3'b101}: instruction_o.op = ariane_pkg::RORW; // rorw
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
{7'b011_0000, 3'b001}: instruction_o.op = ariane_pkg::ROLW; // rolw
{7'b011_0000, 3'b101}: instruction_o.op = ariane_pkg::RORW; // rorw
{7'b011_0000, 3'b001} : instruction_o.op = ariane_pkg::ROLW; // rolw
{7'b011_0000, 3'b101} : instruction_o.op = ariane_pkg::RORW; // rorw

core/decoder.sv Outdated
@@ -851,6 +854,8 @@ module decoder
// Bitwise Shifting
{7'b011_0000, 3'b001}: instruction_o.op = ariane_pkg::ROLW; // rolw
{7'b011_0000, 3'b101}: instruction_o.op = ariane_pkg::RORW; // rorw
// Pack_W
{7'b000_0100, 3'b100}: if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_W; else illegal_instr_bm = 1'b1; //packw
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
{7'b000_0100, 3'b100}: if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_W; else illegal_instr_bm = 1'b1; //packw
{
7'b000_0100, 3'b100
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_W;
else illegal_instr_bm = 1'b1; //packw

@JeanRochCoulon
Copy link
Contributor

Great! To solve the Verible format issues, I suggest to use the command reported in CONTRIBUTING.md.
Do not forget to sign the ECA agreement to bcome cva6 contributor.

Copy link
Contributor

github-actions bot commented Dec 5, 2024

❌ failed run, report available here.

@fatimasaleem fatimasaleem self-requested a review December 5, 2024 13:21
@JeanRochCoulon
Copy link
Contributor

First comments:

  • cv32a60x and cv32a65x do not need the new extension. I suggest to apply the extension the configuration used for FPGA: cv32a6_imac_sv32 and disable the extension for 60x and 65x.
  • In config_pkg.sv files, better is to not define localparam. You can follow the 65x_config_pkg.sv example.
    Cheers

@fatimasaleem
Copy link
Contributor

First comments:

  • cv32a60x and cv32a65x do not need the new extension. I suggest to apply the extension the configuration used for FPGA: cv32a6_imac_sv32 and disable the extension for 60x and 65x.
  • In config_pkg.sv files, better is to not define localparam. You can follow the 65x_config_pkg.sv example.
    Cheers

Thanks @JeanRochCoulon . Sure, we will disable it for cv32a60x and cv32a65x . We only wanted to run the CI with this extension enabled. Since now the CI is almost green, we will disable it for these two.

core/alu.sv Outdated
Comment on lines 398 to 399
result_o = {
{32{fu_data_i.operand_b[15]}}, {fu_data_i.operand_b[15:0]}, {fu_data_i.operand_a[15:0]}
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
result_o = {
{32{fu_data_i.operand_b[15]}}, {fu_data_i.operand_b[15:0]}, {fu_data_i.operand_a[15:0]}
result_o = {
{32{fu_data_i.operand_b[15]}}, {fu_data_i.operand_b[15:0]}, {fu_data_i.operand_a[15:0]}

core/decoder.sv Outdated
// Packing
{
7'b000_0100, 3'b100
} :
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
} :
} :

core/decoder.sv Outdated
Comment on lines 787 to 788
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_H;
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_H;
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_H;

core/decoder.sv Outdated
@@ -849,8 +860,14 @@ module decoder
// Unsigned word Op's
{7'b000_0100, 3'b000}: instruction_o.op = ariane_pkg::ADDUW; // add.uw
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
{7'b000_0100, 3'b000}: instruction_o.op = ariane_pkg::ADDUW; // add.uw
{7'b000_0100, 3'b000} : instruction_o.op = ariane_pkg::ADDUW; // add.uw

core/decoder.sv Outdated
Comment on lines 868 to 869
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_W;
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_W;
} :
if (CVA6Cfg.ZKN) instruction_o.op = ariane_pkg::PACK_W;

Copy link
Contributor

github-actions bot commented Dec 9, 2024

❌ failed run, report available here.

@@ -879,14 +879,14 @@ def load_config(args, cwd):
args.isa = "rv64gch_zba_zbb_zbs_zbc"
elif base in ("cv64a6_imafdc_sv39", "cv64a6_imafdc_sv39_hpdcache", "cv64a6_imafdc_sv39_wb"):
args.mabi = "lp64d"
args.isa = "rv64gc_zba_zbb_zbs_zbc"
Copy link
Contributor

Choose a reason for hiding this comment

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

cv64a6_imafdc_sv39_wb does not support zbkb, this has to be fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

we want to enable this extension for the basic 64-bit config as well, which is cv64a6_imafdc_sv39

@munailwaqar lets only enable it for cv64a6_imafdc_sv39.

elif base == "cv32a60x":
args.mabi = "ilp32"
args.isa = "rv32imc_zba_zbb_zbs_zbc"
Copy link
Contributor

Choose a reason for hiding this comment

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

60x and 65x do not support zbkb

@munailwaqar
Copy link
Author

Currently enabling Zbkb in 2 configs only:

  1. cv32a6_imac_sv32
  2. cv64a6_imafdc_sv39

Copy link
Contributor

❌ failed run, report available here.

core/decoder.sv Outdated
{
7'b000_0100, 3'b100
} : begin
if (instr.instr[24:20] == 5'b00000) instruction_o.op = ariane_pkg::ZEXTH; // Zero Extend Op RV64 encoding
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
if (instr.instr[24:20] == 5'b00000) instruction_o.op = ariane_pkg::ZEXTH; // Zero Extend Op RV64 encoding
if (instr.instr[24:20] == 5'b00000)
instruction_o.op = ariane_pkg::ZEXTH; // Zero Extend Op RV64 encoding

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

✔️ successful run, report available here.

1 similar comment
Copy link
Contributor

✔️ successful run, report available here.

@fatimasaleem
Copy link
Contributor

Hi @JeanRochCoulon, can you please approve this?

@@ -24,7 +24,7 @@ package cva6_config_pkg;
localparam CVA6ConfigZcmpExtEn = 0;
localparam CVA6ConfigAExtEn = 1;
localparam CVA6ConfigHExtEn = 0; // always disabled
localparam CVA6ConfigBExtEn = 0;
localparam CVA6ConfigBExtEn = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This localparam can be removed, because not used

@JeanRochCoulon
Copy link
Contributor

Hello @munailwaqar
Great to have identified riscv-arc-test tests to test the new extension. Can you add these test to the testlist of the 64bit configuration (cv64a6_imafdc_sv39) to insert them into CI and ease the maintenance.

Copy link
Contributor

✔️ successful run, report available here.

@munailwaqar
Copy link
Author

munailwaqar commented Dec 12, 2024

Hello @munailwaqar Great to have identified riscv-arc-test tests to test the new extension. Can you add these test to the testlist of the 64bit configuration (cv64a6_imafdc_sv39) to insert them into CI and ease the maintenance.

@JeanRochCoulon Added the relevant arch tests to the testlist but I also had to enable the Zbkb extension in cv64a6_imafdc_sv39_hpdcache config as the CI runs checks for this config too using the same testlist.

Copy link
Contributor

✔️ successful run, report available here.

@fatimasaleem
Copy link
Contributor

Hi @JeanRochCoulon can you please review this again? Thanks.

@JeanRochCoulon
Copy link
Contributor

Hello @munailwaqar
zip and unzip tests are not included in testlist, is it intentional ?

@munailwaqar
Copy link
Author

Hello @munailwaqar zip and unzip tests are not included in testlist, is it intentional ?

@JeanRochCoulon Yes. zip and unzip instructions are only for 32bit architectures according to the documentation.

@JeanRochCoulon
Copy link
Contributor

I cannot rebase because I have no right on your branch, can you ?

@munailwaqar
Copy link
Author

munailwaqar commented Dec 17, 2024 via email

@munailwaqar
Copy link
Author

@JeanRochCoulon I have rebased the branch.

Copy link
Contributor

❌ failed run, report available here.

@JeanRochCoulon
Copy link
Contributor

LGTM. I am ready to merge.
Can you rebase (again) then, taking back the resulting gate count, adjust the expected gate count ?

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.

3 participants