riscv-software-src / riscv-unified-db

Machine-readable database of the RISC-V specification, and tools to generate various views
Other
6 stars 8 forks source link

Additional parameter for mtvec #127

Open christian-herber-nxp opened 2 hours ago

christian-herber-nxp commented 2 hours ago

According to the ISA manual,

the mtvec register must always be implemented, but can contain a read-only value

There is currently no parameter to allow mtvec.base to be read-only. Thus, I would like to suggest adding:

    MTVEC_BASE_READ_ONLY:
      description: |
        Whether or not the `mtvec.base` field is read only or not.

        Possible values:

        true::
          The `mtvec.base` field is read-only.

        false::
          The `mtvec.base` field is read/write.
      schema:
        type: boolean
james-ball-qualcomm commented 2 hours ago

I had the same impression until Derek showed me something (see screenshot). The db currently assumes that if you only support one value for mtvec.base, then it is read-only. Seems like a reasonable assumption. If you agree, maybe we should just update the description to say this so that others don't get caught by this.

image

dhower-qc commented 2 hours ago

@james-ball-qualcomm, that's for mtvec.MODES. @christian-herber-nxp is referening to mtvec.BASE

dhower-qc commented 2 hours ago

Unfortunately, I think the allowed values are more complicated that just read-only-0 or anything since the field is WARL.

How about this?

    MTVEC_BASE_VALUES:
      description: |
        The supported values in `mtvec.base`. 

        Possible values:

        none::
          `mtvec.base` is read-only-zero.

        width::
          `mtvec.base` supports any value that fits in MTVEC_BASE_WIDTH bits

        custom::
          `mtvec.base` supports a custom set of values. Any attempt to read or write `mtvec.BASE` will
          result in UNPREDICTABLE execution.
      schema:
        type: string
        enum: [none, width, custom]
    MTVEC_BASE_WIDTH:
      description: |
        When MTVEC_BASE_VALUES is 'width', the bit width of supported value in `mtvec.BASE`
      schema:
        type: integer
        minimum: 1
        maximum: 60
      extra_validation: |
        # max width is actually 30 for RV32
        assert MTVEC_BASE_WIDTH <= 30 if MTVEC_BASE_VALUES == "width" && XLEN == 32
james-ball-qualcomm commented 2 hours ago

Oops. Good point. Should we add this parameter then? Makes sense to me.

However, isn't there some general-purpose way to mark a field as read-only? This is going to come up often and I'd hate to think we have to create a parameter for each one.

dhower-qc commented 2 hours ago

Oops. Good point. Should we add this parameter then? Makes sense to me.

However, isn't there some general-purpose way to mark a field as read-only? This is going to come up often and I'd hate to think we have to create a parameter for each one.

Maybe, but it is complicated by WARL (see my comment above)

christian-herber-nxp commented 2 hours ago

There are already parameters that limit the alignment. This makes sense. On top, read-only is the only thing that is needed. This does not have to be read only zero.

The definition of custom above is a bit too unpredictable. There is a very predictable scenario of mtvec being hardwired to a non-zero address. Reading this is not unpredictable.

james-ball-qualcomm commented 1 hour ago

Actually, besides parameters that limit the minimum alignment (affects low-order bits) I'm assuming a valid implementation could have some of the high-order bits read-only-0 to save a few registers (if they didn't need a full 32-bit address space).

I'm thinking that this issue of read-only (not necessarily 0, for example the marchid, mvendorid, mhartid, etc.) and a WARL field that can be fully described by just knowing the number of bits implemented is going to happen so often that we'll want a general-purpose facility for this and not have to create parameters manually for each one. Derek, we've talked about this in the past but I can't remember the outcome of that discussion.

Christian, BTW, the "custom" is Derek's way of saying we don't know how to represent your implementation's behavior (yet) in the db. That's handy for certification since we can say any parameter with a "custom" value is out of scope for certification or means e can't certify your implementation (yet). We'll probably add more choices to parameters over time (or add more parameters) to be able to represent more implementations but we'll be limited to some subset at any given time.