I haven’t checked the vector crypto instructions yet, but I suspect this is already handled.

My understanding of vstart is that pretty much the only thing you can do is save/restore it, and re-execute an interrupted instruction, upon which it should execute as if it hadn't been interrupted.

Software can't safely write arbitrary values of vstart because you don't know which values are valid:

Implementations are permitted to raise illegal-instruction exceptions when attempting to execute a vector instruction with a value of vstart that the implementation can never produce when executing that same instruction with the same vtype setting.

When executing an instruction you must start from vstart:

All vector instructions are defined to begin execution with the element number given in the vstart CSR, leaving earlier elements in the destination vector undisturbed, and to reset the vstart CSR to zero at the end of execution.

Since the current model only produces vstart=0 and always starts from element 0, it is almost compliant with the spec. Except that we don't raise an illegal instruction exception for vstart != 0, so naughty software can still, for example csrw vstart, 10 and then execute a vector store, and it will still start from 0.

The obvious fix is that we require vstart=0 for all instructions.

Then in future we can allow other values but also change the loops from foreach (i in 0... to foreach (i in vstart....

So I reckon for now just change your illegal_vstart function to always return not(assert_vstart(0)).

Note also that the behaviour for when vstart is too large is also unnecessarily vague:

The use of vstart values greater than the largest element index for the current vtype setting is reserved.

It is recommended that implementations trap if vstart is out of bounds. It is not required to trap, as a possible future use of upper vstart bits is to store imprecise trap information.

Btw the thing I mentioned in the meeting about being able to repeat elements was not actually about vstart - I was thinking of this which isn't a problem for us at the moment.

Tim Hutt (@Timmmm) Thanks that, this makes sense!

Since the current model only produces vstart=0 and always starts from element 0, it is almost compliant with the spec. Except that we don't raise an illegal instruction exception for vstart != 0, so naughty software can still, for example csrw vstart, 10 and then execute a vector store, and it will still start from 0.

I was wondering whether it would make sense to start adding support for vstart != 0 for LOAD/STORE RVV instructions soon. I can imagine this is actually something people would like to test and that it could provide some value, especially for load/store operations that cross page boundaries.

I also think it should not be too hard to support. We would probably just need to modify the handful of load/store instructions and start looping from vstart instead of 0 and write vstart when we return an error.

So maybe we do as you suggested for now and require that all instructions trap on vstart != 0, but then later I could create a follow-up PR implementing what I described above, where we would allow vstart != 0 for load/store operations. Thoughts?

Isn't vstart != 0 already supported with the use of get_start_element() and init_masked_*() ? And set_vstart() is already being set before doing a memory operation in case there is an exception? In fact the set_vstart in the load/store loops is the likely cause of #1006.

Prashanth Mundkur (@pmundkur) Ahh yeah, you are right! I took another look at vext_mem_insts.sail, and we already support vstart != 0 for LOAD/STORE instructions.

Ok so I removed the config option for ALU instructions and now always trap if vstart is non-zero.

Other then that I am still not so sure about the naming of the enum values (VS_ALU). Does someone have a better idea? :D

Perhaps _ARITH instead of _ALU? And VSTART_ instead of VS_ to avoid confusion with virtual supervisor?

With regards to act4 testing, we would like to use sail as the reference model to test all our possible test cases. While trapping on vstart != 0 I agree makes a lot of sense from an implementation's perspective, however there are a small subset of normative statements of the spec we would like to exercise that this behaviour would not allow us to.

One example is:

The instructions operate with an effective vector length, evl=NFIELDS*VLEN/EEW, regardless of current settings in vtype and vl. The usual property that no elements are written if vstart ≥ vl does not apply to these instructions. Instead, no elements are written if vstart ≥ evl.

As normally we can run with vstart=0 to satisfy the vstart >= vl condition.

support vstart != 0 for loads addresses most situations but not whole register moves:

The vmvr.v instructions copy whole vector registers (i.e., all VLEN bits) and can copy whole vector register groups. The nr value in the opcode is the number of individual vector registers, NREG, to copy. The instructions operate as if EEW=SEW, EMUL = NREG, effective length evl= EMUL * VLEN/SEW.

jacassidy (@jacassidy) I looked into this and actually started adding it, but I felt it was beginning to balloon the PR and would have made it more complicated to review. Instead, I added back the configuration option for vector arithmetic instructions. With this new configuration option and a follow-up PR, you should be able to test what you're looking for.

Besides that, please let me know if this PR looks good and whether it addresses some of the ATC4 needs.

I haven’t checked the vector crypto instructions yet, but I suspect this is already handled.

I needed to add some checks for Zvbb and Zvbc

Test Results

4 442 tests  ±0   4 442 ✅ ±0   34m 5s ⏱️ - 3m 41s
    1 suites ±0       0 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 3ce00ef. ± Comparison against base commit 0daec7e.

♻️ This comment has been updated with latest results.

Nadime Barhoumi (@nadime15) Understood, thank you for the attempt. Ryan Wolk (@fourth-bit) will be taking over vector, he is currently running this pr against our test suite to confirm it solves any discrepencies and will likely weigh in shortly

It looks like this is giving us the configuration options to match spike and qemu for the arithmetic instructions. But, spike is trapping on scalar move instructions with nonzero vstart. According to riscv/riscv-isa-manual#2564 (comment) this behavior is permitted and not defined as independent of vstart, so it would be nice to also have a configuration option for the behavior of VSTART_SCALAR_MOVE if possible.

But, spike is trapping on scalar move instructions with nonzero vstart. According to riscv/riscv-isa-manual#2564 (comment) this behavior is permitted and not defined as independent of vstart, so it would be nice to also have a configuration option for the behavior of VSTART_SCALAR_MOVE if possible.

I'm having a hard time squaring this with these parts of the spec

VMV.X.S performs its operation even if vstart ≥ vl or vl=0.

The VMV.S.X instruction ... If vstart ≥ vl, no operation is performed and the destination register is not updated. As a consequence, when vl=0, no elements are updated in the destination vector register group, regardless of vstart.

Perhaps I'm misunderstanding something?

We were discussing the part of the spec and had some deliberation. I understand it to be saying that these two instructions either do or dont have additional constraints on vstart, not that their implementation is independent of them. Therefore it would be a legal implication to trap on a vstart that the instruction itself cannot produce. It may be worth opening an issue, or it may be obvious that my interpretation is wrong.

I put up the follow-up PR commit here (https://github.com/nadime15/sail-riscv/tree/fix/vstart-bounds-largest-element-index), stacked on top of this one (its a bit hard to read because it includes the changes made in this PR). It implements the stricter vstart bounds check from #1104.

Right now get_start_element only bounds vstart against a single register's worth of elements, which is too narrow once LMUL > 1, and the follow-up replaces that with the real per instruction element count and moves the validation up front into illegal_vstart.

So this PR gives us the configurable trap policy, and the follow-up closes the bounds-checking side, together they should cover what we need once both land. I kept them separate just to keep each PR focused and easier to review.

It looks like this is giving us the configuration options to match spike and qemu for the arithmetic instructions. But, spike is trapping on scalar move instructions with nonzero vstart. According to riscv/riscv-isa-manual#2564 (comment) this behavior is permitted and not defined as independent of vstart, so it would be nice to also have a configuration option for the behavior of VSTART_SCALAR_MOVE if possible.

I added a new config option (not pushed yet), I just need to double check that the spec really allows it (need to read your linked comment carefully as well as check the spec again).

So, reading Andrew's comment again, it seems that we can actually trap when vstart != 0 simply because these instructions (SCALAR_MOVE) can never produce a non-zero vstart themselves (and implementation are permitted to trap). As he pointed out, a vstart!=0 makes no sense because for vmv.x.s/vfmv.f.s it is ignored by definition anyway, while for vmv.s.x/vfmv.s.f the operation would be masked off when vstart != 0, effectively making it a no-op.

Let's merge this soon so that we can get the out-of-bounds check in as well.