Should vector crypto instructions constraint vd_num/vs1_num/vs2_num align to lmul?

[GuoShibo-cn]

According to zvkned_ext_macros.h, no where constraint vd/vs1 number align to lmul. And overlap just check vs_num == vd_num.

however even with vlen =128, it would be possible to set lmul = 2, register groups could overlap with numbers not the same.

[scottj97]

Can you give a specific example of a case you think is broken today?

[GuoShibo-cn]

Sorry for delay.

Our VLEN is 256 ,this is our simulation result, in the log we use lmul = 4, dst reg_num & src reg_num is not align to lmul, for vector arithmatic instruction this would cause illegal instruction trap, howerver spike just continue the calcualtion.

[aswaterman]

cc @egouriou-rivos (and @chihminchao)

[timhsu404]

Yes, the vector crypto extensions should follow the LMUL alignment requirement.
Though the spec do not describe this behavior explicity, it does not make sense that register can not be the same but the content can still possible to overlap or out of the normal register range, the case v30 with LMUL=4 above.

I will create a PR for it.

[xinyuwang-starfive]

what status of the PR

[aswaterman]

cc @timhsu404

[nibrunieAtSi5]

@timhsu404 any progress on this issue ?

[chihminchao]

@timhsu404 is busy with another internal issue and may not have time on this issue this month.

[timhsu404]

any progress on this issue ?

@nibrunieAtSi5 Not really. I have already done the very first version, but still tracking on some bugs.
The progress is pending for now, I'll keep working on this issue after the urgent job has been done.

[DavidYu360]

Vector crypto instruction VPR alignment rules:
Possible alignment value: 1, 2, 4, 8 (1 for any VPR, 2 for V0, V2, ...)
Prerequisite: VLEN * LMUL >= EGW

1. Single key in .vs inst: max(EGW/VLEN, 1)
2. Others: max(LMUL, 1)

[nibrunie]

I have developed a test case, https://gist.github.com/nibrunie/80a00047dce935011614530d86a829e6, which seems to be passing with flying colors on spike when I would have expected almost every instruction to trap.

[nibrunieAtSi5]

I think the issue is a bit worse than I initially thought: not checking alignment can also lead to segmentation fault as there is no guard to prevent spike from trying to access / write vector registers outside the valid v0 ... v31.

For example, when selecting vd=v31 and LMUL > 1.

The following program seems to seg fault on spike de5094a (Sept 20th):

#include <stdio.h>

// How to build:
//   riscv64-unknown-elf-gcc  -march=rv64gcv_zvkned vcrypto-lmul-alignment-test-case-seg-fault.c
// How to execute:
//   spike --isa=rv64gcv_zvbc_zicntr_zihpm_zvkned <pk-image> a.out

// This program aims at demonstrating that spike is flawed when
// it comes to executing vector crypto instructions with invalid register
// index alignments with respect to LMUL
int main(void) {
  asm volatile (
    // setting LMUL=4
    "vsetivli a1, 16, e32, m4, tu, mu\n"
    "vaesem.vv v20, v31\n" // <=== expected to trap but does not
    "vaesem.vv v31, v11\n" // <=== expected to trap but segfault
    :
    :
    : "a1"
  );
  printf("program should have trapped\n.");
  return 0;
}

[xinyuwang-starfive]

@nibrunie for example lmul=8 vaesdf.vs v8, v11 i think this is overleap, but i think it will not trap in your PR?

[nibrunie]

@nibrunie for example lmul=8 vaesdf.vs v8, v11 i think this is overleap, but i think it will not trap in your PR?

I think you are right @xinyuwang-starfive , this used to be covered by the alignment check (https://github.com/nibrunieAtSi5/riscv-isa-sim/blob/de5094a1a901d77ff44f89b38e00fefa15d4018e/riscv/zvkned_ext_macros.h#L21) but since we are implementing the proper relaxed index constraint for the scalar operand the alignment check is no longer sufficient.

I will try to have a proper / closer look over the week-end and fix my PRs.
Thank you for spotting this @xinyuwang-starfive

[xinyuwang-starfive]

@nibrunie
in vghsh.vv it only check vs2 and vd , i think the vs1 is also need to check align ?

[nibrunie]

@nibrunie in vghsh.vv it only check vs2 and vd , i think the vs1 is also need to check align ?

@xinyuwang-starfive , this was fixed on Sept 25th (b1ce0a3) on the PR opened to address this issue #1815. Is your branch up-to-date ?