Re: [PATCH v2 3/3] target/riscv/translate.c: set vstart_eq_zero in mark_vs_dirty()

[Richard Henderson]

On 2/16/24 03:57, Daniel Henrique Barboza wrote:
> The 'vstart_eq_zero' flag which is used to determine if some insns, like
> vector reductor operations, should SIGILL. At this moment the flag is
> being updated only during cpu_get_tb_cpu_state(), at the start of each
> translation block.
>
> This cadence isn't enough and we're facing situations where a vector
> instruction successfully updated 'vstart' to zero, but the flag was
> still marked as 'false', resulting in a SIGILL because instructions are
> checking the flag.
>
> mark_vs_dirty() is called after any instruction changes Vector CSR
> state, making it a good place to update 'vstart_eq_zero'.
>
> Fixes: 8e1ee1fb57 ("target/riscv: rvv-1.0: add translation-time vector context 
> status")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1976
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/translate.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 177418b2b9..f9ff7b6173 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -652,6 +652,8 @@ static inline void mark_fs_dirty(DisasContext *ctx) { }
>    */
>   static void mark_vs_dirty(DisasContext *ctx)
>   {
> +    TCGLabel *vstart_zero = gen_new_label();
> +    TCGLabel *done = gen_new_label();
>       TCGv tmp;
>   
>       if (ctx->mstatus_vs != EXT_STATUS_DIRTY) {
> @@ -669,6 +671,24 @@ static void mark_vs_dirty(DisasContext *ctx)
>               tcg_gen_st_tl(tmp, tcg_env, offsetof(CPURISCVState, mstatus_hs));
>           }
>       }
> +
> +    /*
> +     * We can safely make 'vl_eq_vlmax = false' if we marked
> +     * VS as dirty with non-zero 'vstart', i.e. there's a fault
> +     * to be handled. If 'vstart' is zero then we should retain
> +     * the existing 'vl_eq_vlmax' - it'll be recalculated on the
> +     * start of the next TB or during vset{i}vl{i} (that forces a
> +     * TB end).
> +     */
> +    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vstart, 0, vstart_zero);
> +    ctx->vstart_eq_zero = false;
> +    ctx->vl_eq_vlmax = false;
> +    tcg_gen_br(done);
> +
> +    gen_set_label(vstart_zero);
> +    ctx->vstart_eq_zero = true;
> +
> +    gen_set_label(done);

This is very confused, apparently generating code to test vstart at runtime, and then set 
some translation time variables in the branches.

Afaik, the only way vstart != 0 is an explicit set to the CSR or exiting a load via 
exception.  Therefore you have no need to have any sort of brcond here -- just set
ctx->vstart_eq_zero = true.

Also, you need to move the ifdefs from around mark_vs_dirty, because it is now relevant to 
user-only.

It may be worth a rename, because it does more than mark vs dirty, and therefore is 
different than mark_fs_dirty.

You need to review all instances of

    TCGLabel *over = gen_new_label();
    tcg_gen_brcond_tl(TCG_COND_GEU, cpu_vstart, cpu_vl, over);
...
    gen_set_label(over);
    return true;

because this *should have* set vstart = 0.  With vstart < vl, this is done in the helper 
function, but every place using a conditional branch needs attention.


r~