regtool: reg_top triggers write error when not writing to a permitted byte, instead of when writing to a forbidden byte

[cousteaulecommandant]

https://github.com/lowRISC/opentitan/blob/e9925bdf1464e9cec8ab8931c108cee4acb2709b/util/reggen/reg_top.sv.tpl#L655-L659

Shouldn't this code check |(reg_be & ~PERMIT[i]), not |(PERMIT[i] & ~reg_be)? I.e., "if a byte is being written (reg_be[j] = 1) and it is not permitted (PERMIT[i][j] = 0, or ~PERMIT[i][j] = 1), then there's an error."

Currently the error signal seems to be activated when any of the permitted bytes is not being written, instead of when any of the non-permitted bytes is being written.

[rswarbrick]

I think I agree. I picked a module (pretty much at random) and looked at otbn_reg_top.sv. The code there looks like

    wr_err = (reg_we &
              ((addr_hit[ 0] & (|(OTBN_PERMIT[ 0] & ~reg_be))) |
...

Here OTBN_PERMIT is 4'b0001. The interesting bit ends up looking like 4'b0001 & ~reg_be, which (as you say) is nonzero if reg_be[0] is zero (rather than being nonzero if reg_be[3:1] is nonzero, which I think was the intended behaviour).

Embarrassingly, this change goes back to ce8e393d4b2, which I made in 2021. Before that change, we had lines like this:

    if (addr_hit[0] && reg_we && (OTBN_PERMIT[0] != (OTBN_PERMIT[0] & reg_be))) wr_err = 1'b1 ;

But I think this had the same behaviour. The (OTBN_PERMIT[0] != (OTBN_PERMIT[0] & reg_be)) bit is nonzero unless every bit that is set in OTBN_PERMIT[0] is also set in reg_be.

That's even more concerning (and not just my change): it goes back all the way to 51461cd27e0f7bea13ae61e05e6b0ed3da2a5da7 in 2019. Crikey!

Pinging a couple of designers to see whether we've interpreted this correctly. @vogelpi, @andreaskurth: comments?

[cousteaulecommandant]

On second thought, the current behavior is: "If a write transaction is made that does NOT write all writable bytes, this is an error". This might actually make sense, since apparently you cannot make sub-word writes to registers (writing to a single byte doesn't honor the reg_be signal, and the whole register is written (even if it consists of 4 byte-sized fields), possibly writing garbage/zeros on the "unwritten" bytes), so writing to single bytes isn't correctly implemented right now. I don't know if this is the intended behavior (write to the whole register and ignore the byte enable signal); if it is, it makes sense to make partial writes be considered erroneous, whereas writes to "forbidden" bytes (e.g. on a 24-bit register, where you may want to write the whole register at once as a 32-bit word rather than writing 1 byte 3 times) may simply be ignored. But if this behavior is desired, it should be properly documented, as the comment above currently doesn't match what the code does.

Overall, I like the idea of being able to write to byte-sized fields so this would be nice, but I see how it could get complicated with fields that do not align to byte boundaries or span multiple bytes.

[a-will]

CSRs do not support writes that are smaller than the full width, and this is intentional. To keep the design and implementation simpler, we define the full CSR as the unit of atomicity, not the fields.

Memory interfaces, on the other hand, can support byte enables.

[rswarbrick]

Oh, that makes sense. Do you know why we have the byte-enable signal at all?

[a-will]

Oh, that makes sense. Do you know why we have the byte-enable signal at all?

They'd be needed to enforce that full-width requirement in the check, where "full width" actually means "the span of bytes from the LSB to the byte that includes the MSB of the most-significant field, then rounded up as needed to reach a power of 2"--This is how the *_PERMIT values are defined.

I can't say I know precisely what motivated this more flexible receiver, though. I hadn't joined the project yet when that was decided. :)

[cousteaulecommandant]

OK, so the PERMIT bits aren't really showing which bytes CAN be written, but which bytes MUST be written, correct? (In that case, I guess the code can stay like that but the comment above should be rectified, and perhaps you may consider renaming PERMIT to REQUIRE to avoid confusion).

It may also be a good idea to add a note in the regtool documentation to explicitly clarify that partial writes to a register (even a multi-field one) are not permitted, and if you want to do such a thing you should consider using a mini-window instead, or making the register external. (I had a look at the docs and didn't see anything in that regard, although I didn't look too thoroughly.)

[andreaskurth]

I think the implementation is correct in that it raises an error for writes that don't modify all writable bytes of a CSR. This allows for a simpler system design (as @a-will mentioned) and is part of a security mechanism to deter fault injection attacks that intend to skip writes to certain CSR fields. (CSRs cannot always be read back to ensure that a write has happened as intended.)

This behavior is documented in reggen -> Error responses, but I agree that documentation and code comments can be improved to clarify this. I suggest the following:

• Add a paragraph to the regtool documentation that explains this design and its rationale.
• Clarify the comment in reg_top.sv.tpl.
• Consider renaming the PERMIT constant.

[rswarbrick]

Thanks for the careful responses, @andreaskurth and @a-will. This is a relief and I agree with the follow-up tasks. I'll try to take them on and tidy things up (if only to reassure myself!) in the next couple of days.

[rswarbrick]

I've just filed #22931, which fixes the main issue here (which turns out to have been caused by a misunderstanding I had back in 2021. Oops!)

I probably wouldn't advocate renaming the PERMIT constant. We've checked in that name all over the repository and the change probably doesn't matter, so I'm not really in favour of the churn.

[cousteaulecommandant]

Good, thanks!

Maybe the documentation could do with an explanation at the beginning explaining that registers can contain multiple fields, and that all of them must be written at once, since the documentation never explains what fields are, and just goes straight to how to define them. Something like:

+Registers may be subdivided into multiple fields, similarly to packed structs in SystemVerilog or bit-fields in C. All writable fields in a register must be written simultaneously.

  Register fields are tagged using the swaccess key[...]

---

I probably wouldn't advocate renaming the PERMIT constant. We've checked in that name all over the repository and the change probably doesn't matter, so I'm not really in favour of the churn.

Sounds reasonable, but it might help to clarify that in the comment above the definition of *_PERMIT. Like "note that it says 'PERMIT' but actually means 'required'", or at least "All bytes must be written for the write to be permitted" (which kinda justifies naming it "permit").