Re: [PATCH v6 1/9] trans_rvv.c.inc: mark_vs_dirty() before loads and stores

[Alistair Francis]

On Thu, Feb 22, 2024 at 7:34 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> While discussing a problem with how we're (not) setting vstart_eq_zero
> Richard had the following to say w.r.t the conditional mark_vs_dirty()
> calls on load/store functions [1]:
>
> "I think it's required to have stores set dirty unconditionally, before
> the operation.
>
> Consider a store that traps on the 2nd element, leaving vstart = 2, and
> exiting to the main loop via exception. The exception enters the kernel
> page fault handler. The kernel may need to fault in the page for the
> process, and in the meantime task switch.
>
> If vs dirty is not already set, the kernel won't know to save vector
> state on task switch."
>
> Do a mark_vs_dirty() before both loads and stores.
>
> [1] 
> https://lore.kernel.org/qemu-riscv/72c7503b-0f43-44b8-aa82-fbafed2aac0c@linaro.org/
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/insn_trans/trans_rvv.c.inc | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc 
> b/target/riscv/insn_trans/trans_rvv.c.inc
> index 9e101ab434..7a98f1caa6 100644
> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> @@ -636,11 +636,9 @@ static bool ldst_us_trans(uint32_t vd, uint32_t rs1, 
> uint32_t data,
>      tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
>      tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
>
> -    fn(dest, mask, base, tcg_env, desc);
> +    mark_vs_dirty(s);
>
> -    if (!is_store) {
> -        mark_vs_dirty(s);
> -    }
> +    fn(dest, mask, base, tcg_env, desc);
>
>      gen_set_label(over);
>      return true;
> @@ -797,11 +795,9 @@ static bool ldst_stride_trans(uint32_t vd, uint32_t rs1, 
> uint32_t rs2,
>      tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
>      tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
>
> -    fn(dest, mask, base, stride, tcg_env, desc);
> +    mark_vs_dirty(s);
>
> -    if (!is_store) {
> -        mark_vs_dirty(s);
> -    }
> +    fn(dest, mask, base, stride, tcg_env, desc);
>
>      gen_set_label(over);
>      return true;
> @@ -904,11 +900,9 @@ static bool ldst_index_trans(uint32_t vd, uint32_t rs1, 
> uint32_t vs2,
>      tcg_gen_addi_ptr(index, tcg_env, vreg_ofs(s, vs2));
>      tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
>
> -    fn(dest, mask, base, index, tcg_env, desc);
> +    mark_vs_dirty(s);
>
> -    if (!is_store) {
> -        mark_vs_dirty(s);
> -    }
> +    fn(dest, mask, base, index, tcg_env, desc);
>
>      gen_set_label(over);
>      return true;
> @@ -1102,11 +1096,10 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t 
> rs1, uint32_t nf,
>      base = get_gpr(s, rs1, EXT_NONE);
>      tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
>
> +    mark_vs_dirty(s);
> +
>      fn(dest, base, tcg_env, desc);
>
> -    if (!is_store) {
> -        mark_vs_dirty(s);
> -    }
>      gen_set_label(over);
>
>      return true;
> --
> 2.43.2
>
>