[otbn] OTBN instruction set for gnu-as #1

andrea-caforio · 2026-02-02T07:43:41Z

I call it the + extension. Excluding boilerplate, it's around 200 lines of logic. Let me know what
you think.

Adds the ability to compile OTBN programs with the GNU assembler. Use the -march=rv32+ option to instrument the compiler.

./as -mno-relax -mno-arch-attr -mabi=ilp32 -march=rv32+ -g sw/otbn/crypto/mul.s

Adds the ability to compile OTBN programs with the GNU assembler. Use the `-march=rv32+` option to instrument the compiler. ``` ./as -mno-relax -mno-arch-attr -mabi=ilp32 -march=rv32+ -g sw/otbn/crypto/mul.s ``` Signed-off-by: Andrea Caforio <andrea.caforio@lowrisc.org>

andrea-caforio · 2026-02-02T07:44:53Z

@rswarbrick @ziuziakowska

rswarbrick · 2026-02-02T09:27:24Z

I haven't reviewed this carefully, but it seems kind of reasonable to me. Presumably, your motivation is to avoid needing the (slightly hacky) otbn-as Python script that I wrote for OpenTitan? One downside is that you end up requiring a particular toolchain if you want to assemble programs.

Is there a reason that it's worth paying this cost?

andrea-caforio · 2026-02-02T10:57:09Z

For us it would be nice to have access to the full gnu-as feature set for OTBN code. It would allow us
to write cryptographic code that is easier to write, read and maintain and could be made faster. Some
features that are not possible with otbn_as.py:

Constants (+expressions) for all instructions.
Macros are completely disabled.
- In general, all the .* commands for all OTBN instructions.
File inclusions.

For reference, ZeroRISC hacked together some partial constant and macro support in otbn_as.py that
allowed them to write a pretty concise ML-DSA implementation. It's very brittle, I think it is easier
to simply extend the binutils like in this PR.

rswarbrick · 2026-02-02T14:02:36Z

That sounds sensible to me. I assume we're planning to add a dependency on this binutils and delete otbn_as once this lands? I REALLY don't want to have two different paths

rswarbrick

I've noticed a few things that need sorting out. Annoyingly, this includes inserting TAB characters (eugh!)

A proper code review probably needs someone who understands tooling better than I do. @luismarques or @ziuziakowska: help!

rswarbrick · 2026-02-02T14:02:54Z

gas/config/tc-riscv.c

  CSR_CLASS_NONE,

  CSR_CLASS_I,
+  CSR_CLASS_OTBN, /* OTBN */


Nit: Spacing

rswarbrick · 2026-02-02T14:04:01Z

gas/config/tc-riscv.c

+     * OTBN
+     */
+
+   case '+':


I think this file uses tabs (boo! hiss!). We should probably match.

gas/config/tc-riscv.c

rswarbrick · 2026-02-02T14:06:37Z

include/opcode/riscv-opc.h


 #endif /* DECLARE_INSN */
 #ifdef DECLARE_CSR
+DECLARE_CSR(FG0, CSR_FG0, CSR_CLASS_OTBN, PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)


Can you declare the WSRs too? (Missing support for CSRs/WSRs was an ugly hole in my Python hack!)

luismarques · 2026-02-02T14:22:44Z

@andrea-caforio what are you using for testing this? I assume we have a way to easily and (somewhat) exahaustively test this against otbn_as.py?

Regarding the broader PR, adding such support to the toolchain proper makes sense to me. I remember people occasionally struggling with some limitations of otbn_as.py, even before the recent motivations @andrea-caforio mentioned.

I REALLY don't want to have two different paths

If we have a way to easily test the two OTBN assemblers and fairly exhaustively test them, that doesn't sound too bad to me. I imagine it might be useful to keep the simple Python variant around for a while more, in case people need to test something with an older version of binutils, without this downstream OBTN support.

andrea-caforio · 2026-02-03T09:26:21Z

@andrea-caforio what are you using for testing this? I assume we have a way to easily and (somewhat) exahaustively test this against otbn_as.py?

Regarding the broader PR, adding such support to the toolchain proper makes sense to me. I remember people occasionally struggling with some limitations of otbn_as.py, even before the recent motivations @andrea-caforio mentioned.

I REALLY don't want to have two different paths

If we have a way to easily test the two OTBN assemblers and fairly exhaustively test them, that doesn't sound too bad to me. I imagine it might be useful to keep the simple Python variant around for a while more, in case people need to test something with an older version of binutils, without this downstream OBTN support.

Currently, I've been compiling existing OTBN apps with this and checking whether the generated binaries are
identical to what otbn_as.py produces. This could be automatized.

Another option would be to use the built-in test rig of gnu-as that allows for verifying generated binaries.
I have to check it out.

ziuziakowska · 2026-02-03T16:42:19Z

I haven't reviewed this carefully, but it seems kind of reasonable to me. Presumably, your motivation is to avoid needing the (slightly hacky) otbn-as Python script that I wrote for OpenTitan? One downside is that you end up requiring a particular toolchain if you want to assemble programs.

Is there a reason that it's worth paying this cost?

I haven't had to use/interact with OTBN so I do not know where the tooling used lives, however the plan for this repository is to be the new Binutils source for the Ibex toolchain (once we upgrade LLVM and do a new release featuring both of these), so this will make its way into the internal toolchain build that Opentitan pulls in.

ziuziakowska

Thank you for this PR! 🙏 I've had a read through it and the documentation for OTBN as I have never interacted with it, and I think it is a good idea to add support for this into the assembler. Most of the comments on the source are just formatting nits (GNU formatting does use tabs annoyingly...), but I'll put everything else here.

I think + as an extension name might not be ideal, although we are stepping into weird territory here - as I understand the OTBN instruction set is more of a new "base" ISA that is incompatible with rv32i, etc, than an "extension" of those, though adding a new base ISA is probably too intrusive of a change to bother with. I would prefer the letter o for OTBN.

I would also ask that you split these changes across multiple commits. This repository originally started as a from scratch re-implementation of the Bitmanip patch for 2.35 on 2.44, as the code had diverged way too much for the patch to apply correctly at all. For that reason I have tried to make all my changes as granular as possible so that we can rebase much easier on newer versions in the future, and added rationale in the commit messages for anyone not familiar with the codebase.

It would also be great if you added some test cases. I don't remember off the top of my head how to run the tests, but you can see in 6c125e1 how to add an assembler test case by providing a test file and an expected output regex file. Just assembling each opcode once should suffice. You will also need to add the extension name and version to the test that checks the output of -march=help like here: f277b05.

ziuziakowska · 2026-02-03T16:02:19Z

gas/config/tc-riscv.c

-	 and set fx_done for -mno-relax.  */
+	 and set fx_done for -mno-relax.
+   If we are compiling OTBN code, do not add the offset to the PC. */


Nit: indentation.

ziuziakowska · 2026-02-03T16:06:13Z

opcodes/riscv-opc.c

+  "w0",   "w1",   "w2",   "w3",   "w4",   "w5",   "w6",   "w7",
+  "w8",   "w9",   "w10",  "w11",  "w12",  "w13",  "w14",  "w15",
+  "w16",  "w17",  "w18",  "w19",  "w20",  "w21",  "w22",  "w23",
+  "w24",  "w25",  "w26",  "w27",  "w28",  "w29",  "w30",  "w31"
+};
+
+const char riscv_wdr_q_names_numeric[NWDRQ][NRC] =
+{
+   "w0.0",   "w0.1",  "w0.2",  "w0.3",
+   "w1.0",   "w1.1",  "w1.2",  "w1.3",
+   "w2.0",   "w2.1",  "w2.2",  "w2.3",
+   "w3.0",   "w3.1",  "w3.2",  "w3.3",
+   "w4.0",   "w4.1",  "w4.2",  "w4.3",
+   "w5.0",   "w5.1",  "w5.2",  "w5.3",
+   "w6.0",   "w6.1",  "w6.2",  "w6.3",
+   "w7.0",   "w7.1",  "w7.2",  "w7.3",
+   "w8.0",   "w8.1",  "w8.2",  "w8.3",
+   "w9.0",   "w9.1",  "w9.2",  "w9.3",
+  "w10.0",  "w10.1", "w10.2", "w10.3",
+  "w11.0",  "w11.1", "w11.2", "w11.3",
+  "w12.0",  "w12.1", "w12.2", "w12.3",
+  "w13.0",  "w13.1", "w13.2", "w13.3",
+  "w14.0",  "w14.1", "w14.2", "w14.3",
+  "w15.0",  "w15.1", "w15.2", "w15.3",
+  "w16.0",  "w16.1", "w16.2", "w16.3",
+  "w17.0",  "w17.1", "w17.2", "w17.3",
+  "w18.0",  "w18.1", "w18.2", "w18.3",
+  "w19.0",  "w19.1", "w19.2", "w19.3",
+  "w20.0",  "w20.1", "w20.2", "w20.3",
+  "w21.0",  "w21.1", "w21.2", "w21.3",
+  "w22.0",  "w22.1", "w22.2", "w22.3",
+  "w23.0",  "w23.1", "w23.2", "w23.3",
+  "w24.0",  "w24.1", "w24.2", "w24.3",
+  "w25.0",  "w25.1", "w25.2", "w25.3",
+  "w26.0",  "w26.1", "w26.2", "w26.3",
+  "w27.0",  "w27.1", "w27.2", "w27.3",
+  "w28.0",  "w28.1", "w28.2", "w28.3",
+  "w29.0",  "w29.1", "w29.2", "w29.3",
+  "w30.0",  "w30.1", "w30.2", "w30.3",
+  "w31.0",  "w31.1", "w31.2", "w31.3"
+};
+
+const char riscv_wdr_h_names_numeric[NWDRH][NRC] =
+{
+   "w0.L",   "w0.U",
+   "w1.L",   "w1.U",
+   "w2.L",   "w2.U",
+   "w3.L",   "w3.U",
+   "w4.L",   "w4.U",
+   "w5.L",   "w5.U",
+   "w6.L",   "w6.U",
+   "w7.L",   "w7.U",
+   "w8.L",   "w8.U",
+   "w9.L",   "w9.U",
+  "w10.L",  "w10.U",
+  "w11.L",  "w11.U",
+  "w12.L",  "w12.U",
+  "w13.L",  "w13.U",
+  "w14.L",  "w14.U",
+  "w15.L",  "w15.U",
+  "w16.L",  "w16.U",
+  "w17.L",  "w17.U",
+  "w18.L",  "w18.U",
+  "w19.L",  "w19.U",
+  "w20.L",  "w20.U",
+  "w21.L",  "w21.U",
+  "w22.L",  "w22.U",
+  "w23.L",  "w23.U",
+  "w24.L",  "w24.U",
+  "w25.L",  "w25.U",
+  "w26.L",  "w26.U",
+  "w27.L",  "w27.U",
+  "w28.L",  "w28.U",
+  "w29.L",  "w29.U",
+  "w30.L",  "w30.U",
+  "w31.L",  "w31.U"


Nit: Extra spacing in between some of the columns.

ziuziakowska · 2026-02-03T16:28:12Z

opcodes/riscv-opc.c

 {"srl",         0, INSN_CLASS_C, "Cs,Cw,C>",  MATCH_C_SRLI, MASK_C_SRLI, match_srxi_as_c_srxi, INSN_ALIAS },
 {"srl",         0, INSN_CLASS_I, "d,s,t",     MATCH_SRL, MASK_SRL, match_opcode, 0 },
 {"srl",         0, INSN_CLASS_I, "d,s,>",     MATCH_SRLI, MASK_SRLI, match_opcode, INSN_ALIAS },
+{"srl",         0, INSN_CLASS_I, "d,s,t",     MATCH_SRL, MASK_SRL, match_opcode, 0 },


A lot of these should be INSN_CLASS_OTBN, otherwise they are just duplicated.

ziuziakowska · 2026-02-03T16:28:31Z

opcodes/riscv-opc.c

 {"sw",          0, INSN_CLASS_C, "CV,CM(Cc)", MATCH_C_SWSP, MASK_C_SWSP, match_opcode, INSN_ALIAS|INSN_DREF|INSN_4_BYTE },
 {"sw",          0, INSN_CLASS_C, "Ct,Ck(Cs)", MATCH_C_SW, MASK_C_SW, match_opcode, INSN_ALIAS|INSN_DREF|INSN_4_BYTE },
 {"sw",          0, INSN_CLASS_I, "t,q(s)",    MATCH_SW, MASK_SW, match_opcode, INSN_DREF|INSN_4_BYTE },
+{"sw",          0, INSN_CLASS_OTBN, "t,q(s)",    MATCH_SW, MASK_SW, match_opcode, INSN_DREF|INSN_4_BYTE },


Nit: Indentation

ziuziakowska · 2026-02-03T16:28:57Z

opcodes/riscv-opc.c

+{"bn.wsrr",         0, INSN_CLASS_OTBN, "+a,+E",          MATCH_BN_WSRR, MASK_BN_WSRR, match_opcode, 0 },
+{"bn.wsrw",         0, INSN_CLASS_OTBN, "+E,+b",          MATCH_BN_WSRW, MASK_BN_WSRW, match_opcode, 0 },
+
+{"loop",        0, INSN_CLASS_OTBN, "s,+B",       MATCH_LOOP, MASK_LOOP, match_opcode, 0 },


Nit: Indent

ziuziakowska · 2026-02-03T16:29:40Z

bfd/elfxx-riscv.c

      table = riscv_supported_std_ext;
    }

+


Nit: Extra newline

ziuziakowska · 2026-02-03T16:30:26Z

bfd/elfxx-riscv.c

-  if (*p != 'e' && *p != 'i' && *p != 'g')
+  if (*p != 'e' && *p != 'i' && *p != 'g' && *p != '+')
    {
      rps->error_handler
 	(_("%s: first ISA extension must be `e', `i' or `g'"),


The error message below should also include this new letter.

ziuziakowska · 2026-02-03T16:31:05Z

include/opcode/riscv-opc.h

+/*
+ * OTBN-only instructions.
+ */


This comment is short enough to fit on one line.

luismarques · 2026-02-03T17:52:35Z

I think + as an extension name might not be ideal, although we are stepping into weird territory here - as I understand the OTBN instruction set is more of a new "base" ISA that is incompatible with rv32i, etc, than an "extension" of those, though adding a new base ISA is probably too intrusive of a change to bother with. I would prefer the letter o for OTBN.

We should use the x suffix for vendor extensions. For instance, xlowriscotbn or lrotbn.

etterli · 2026-02-04T07:46:24Z

I'm in favor of adding proper OTBN support to the assembler as it helps a lot to simplify OTBN programming.

However, this extension will duplicate the instruction definition. As of now, the OTBN instructions are defined in yml files (hw/ip/otbn/data/bignum-insns.yml and hw/ip/otbn/data/base-insns.yml). These files are used for documentation as well as DV (the random instruction generator (RIG) generates the test programs based on these). When we add this extension, the instructions are defined in the extension as well. So we have to maintain two definitions and more importantly ensure that these definitions are in sync. This seems a little bit cumbersome and prone to errors.

Is there a smart way of ensuring this?

ziuziakowska · 2026-02-04T10:48:22Z

We should use the x suffix for vendor extensions. For instance, xlowriscotbn or lrotbn.

I think either of these is fine. I think the spec doesn't have provisions for a custom "base" ISA so we should treat it as a "extension" like this.

ziuziakowska · 2026-02-04T11:06:21Z

I'm in favor of adding proper OTBN support to the assembler as it helps a lot to simplify OTBN programming.

However, this extension will duplicate the instruction definition. As of now, the OTBN instructions are defined in yml files (hw/ip/otbn/data/bignum-insns.yml and hw/ip/otbn/data/base-insns.yml). These files are used for documentation as well as DV (the random instruction generator (RIG) generates the test programs based on these). When we add this extension, the instructions are defined in the extension as well. So we have to maintain two definitions and more importantly ensure that these definitions are in sync. This seems a little bit cumbersome and prone to errors.

Is there a smart way of ensuring this?

For the bitmanip work, I defined the instructions in a local copy of RISC-V opcodes, which uses the encoding definition to generate these mask and match constants in the format used by binutils, OpenSBI, etc. The syntax for those files looks like this:

bext     rd rs1 rs2 31..25=0b0000100 14..12=0b110 6..2=0b01100 1..0=3
bdep     rd rs1 rs2 31..25=0b0100100 14..12=0b110 6..2=0b01100 1..0=3

With the following output:

#define MATCH_BDEP 0x48006033
#define MASK_BDEP 0xfe00707f
#define MATCH_BEXT 0x8006033
#define MASK_BEXT 0xfe00707f

#ifdef DECLARE_INSN
DECLARE_INSN(bdep, MATCH_BDEP, MASK_BDEP)
DECLARE_INSN(bext, MATCH_BEXT, MASK_BEXT)
#endif

The yaml data for the instructions seems to look quite similar, and we could make some (maybe intrusive) changes to implement the OTBN operand types and use a script to map the yaml to the RISC-V opcodes format.


- mnemonic: bn.addc
  synopsis: Add with Carry
  operands: *bn-add-operands
  syntax: *bn-add-syntax
  doc: |
    Adds two WDR values and the Carry flag value, writes the result to the
    destination WDR, and updates the flags. The content of the second source
    WDR can be shifted by an unsigned immediate before it is consumed by the
    operation.
  errs: []
  iflow:
    - to: [wrd, flags-all]
      from: [wrs1, wrs2, flags-c]
  encoding:
    scheme: bnaf
    mapping:
      fg: flag_group
      shift_type: shift_type
      shift_bits: shift_bits
      wrs2: wrs2
      wrs1: wrs1
      funct3: b010
      wrd: wrd

andrea-caforio · 2026-02-06T08:47:25Z

Thank @rswarbrick @luismarques @ziuziakowska @etterli for the reviews. I really
appreciate it. I'm not sure when I get around to implementing it because PQC is
a more pressing task for me currently.

In any case, I will try to come up with an appropriate testing framework to properly
verify the correctness of the patch.

rswarbrick reviewed Feb 2, 2026

View reviewed changes

luismarques requested a review from ziuziakowska February 3, 2026 15:48

ziuziakowska requested changes Feb 3, 2026

View reviewed changes

andrea-caforio mentioned this pull request Feb 6, 2026

[bazel] Update binutils to 2.44 lowRISC/opentitan#29246

Open

[otbn] OTBN instruction set for gnu-as #1

Are you sure you want to change the base?

[otbn] OTBN instruction set for gnu-as #1

Uh oh!

Conversation

andrea-caforio commented Feb 2, 2026

Uh oh!

andrea-caforio commented Feb 2, 2026

Uh oh!

rswarbrick commented Feb 2, 2026

Uh oh!

andrea-caforio commented Feb 2, 2026

Uh oh!

rswarbrick commented Feb 2, 2026

Uh oh!

rswarbrick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luismarques commented Feb 2, 2026

Uh oh!

andrea-caforio commented Feb 3, 2026

Uh oh!

ziuziakowska commented Feb 3, 2026

Uh oh!

ziuziakowska left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luismarques commented Feb 3, 2026

Uh oh!

etterli commented Feb 4, 2026

Uh oh!

ziuziakowska commented Feb 4, 2026

Uh oh!

ziuziakowska commented Feb 4, 2026

Uh oh!

andrea-caforio commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rswarbrick left a comment •

edited

Loading