Re: [PATCH 4/5] [RISCV_PM] Add address masking functions required for RISC-V Pointer Masking extension

[Richard Henderson <richard.henderson@linaro.org>]

On 10/14/20 10:01 AM, Alexey Baturo wrote:
> +static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
> +{
> +    gen_pm_adjust_address(s, addr, addr);
> +    return addr;
> +}

This function is unused in this patch, which means the series as a whole is
non-bisectable.

Rather than merge the two, I suggest adding a stub version of this function to
patch 5, and then swap patch 4 and patch 5.  So you will add uses of
apply_pointer_masking without actually implementing it yet.  Which should be fine.

> @@ -800,8 +836,36 @@ static void riscv_tr_init_disas_context
>      } else {
>          ctx->virt_enabled = false;
>      }
> +    if (riscv_has_ext(env, RVJ)) {
> +        switch (env->priv) {
> +        case PRV_U:
> +            ctx->pm_enabled = get_field(env->mmte, UMTE_U_PM_ENABLE);
> +            ctx->pm_mask = env->upmmask;
> +            ctx->pm_base = env->upmbase;
> +            break;
> +        case PRV_S:
> +            ctx->pm_enabled = get_field(env->mmte, SMTE_S_PM_ENABLE);
> +            ctx->pm_mask = env->spmmask;
> +            ctx->pm_base = env->spmbase;
> +            break;
> +        case PRV_M:
> +            ctx->pm_enabled = get_field(env->mmte, MMTE_M_PM_ENABLE);
> +            ctx->pm_mask = env->mpmmask;

You can't read env like this.

This bakes in values from ENV without adding any way to verify that those
values are still current.

The one thing that you must bake into the generated code is the state of
PM_ENABLE.  Anything else would penalize normal risc-v emulation.

You do that in cpu_get_tb_cpu_state().  Allocate one bit to hold
the current state of the flag.  E.g.

FIELD(TB_FLAGS, PM_ENABLED, 9, 1)

then fill it in from the correct mmte bit for priv (which itself is encoded by
cpu_mmu_index()).

Except for special cases, the mask and base variables cannot be placed into
TB_FLAGS.  For now, I think it's best to ignore the special cases and implement
them all as tcg globals.  Which means that we do *not* bake in a particular
value, but instead read the value from env at runtime.

So, in riscv_translate_init, you create new globals for each of the mask and
base.  In riscv_tr_init_disas_context you examine priv (via mmu_index) and
assign one pair of the globals to DisasContext, so that you don't have to keep
looking them up.

Then you have

static void gen_pm_adjust_address(DisasContext *s,
                                  TCGv_i64 dst,
                                  TCGv_i64 src)
{
    if (s->pm_enabled == 0) {
        /* Load unmodified address */
        tcg_gen_mov_i64(dst, src);
    } else {
        tcg_gen_andc_i64(dst, src, s->pm_mask);
        tcg_gen_or_i64(dst, dst, s->pm_base);
    }
}


r~