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

[Alexey Baturo]

Hi folks,

I've sent new v2 patches where, I hope, I managed to address all the comments and suggestions that were provided.

Thanks!

ср, 14 окт. 2020 г. в 23:10, Alexey Baturo <baturo.alexey@gmail.com>:

> I suggest adding a stub version of this function to patch 5, and then swap patch 4 and patch 5.

Thanks, will do.

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

If I correctly get your idea, you're talking about the situation, when DisasContext was initialized with some values, which at some point got changed, so this could lead to incorrect address masking. I tried to handle this situation by dropping the translation cache in case different values are written in any of the PM CSRs, which I assumed would lead to refilling the DIsasContext structure.

This is obviously not the best way to do it, since it may lead to performance degradation in some cases, so let me process your suggestion and try to implement it.

Thanks!

ср, 14 окт. 2020 г. в 22:19, 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~