diff mbox series

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

Message ID 20201014170159.26932-5-space.monkey.delivers@gmail.com
State New
Headers show
Series [1/5,RISCV_PM] Add J-extension into RISC-V | expand

Commit Message

Alexey Baturo Oct. 14, 2020, 5:01 p.m. UTC 
From: Anatoly Parshintsev <kupokupokupopo@gmail.com>

Signed-off-by: Anatoly Parshintsev <kupokupokupopo@gmail.com>
---
 target/riscv/translate.c | 65 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

Comments

Richard Henderson Oct. 14, 2020, 7:19 p.m. UTC | #1 
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~
Alexey Baturo Oct. 14, 2020, 8:10 p.m. UTC | #2 
> 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~
>
Alexey Baturo Oct. 15, 2020, 3:23 p.m. UTC | #3 
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~
>>
>
diff mbox series

Patch

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 79dca2291b..338a967e0c 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -63,6 +63,10 @@  typedef struct DisasContext {
     uint16_t vlen;
     uint16_t mlen;
     bool vl_eq_vlmax;
+    /* PointerMasking extension */
+    uint8_t pm_enabled;
+    target_ulong pm_mask;
+    target_ulong pm_base;
 } DisasContext;
 
 #ifdef TARGET_RISCV64
@@ -90,6 +94,38 @@  static inline bool has_ext(DisasContext *ctx, uint32_t ext)
     return ctx->misa & ext;
 }
 
+/* Generates address adjustment for PointerMasking */
+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 {
+        TCGv_i64 mask_neg = tcg_const_i64(~s->pm_mask);
+        TCGv_i64 base     = tcg_const_i64(s->pm_base);
+        /* calculate (addr & ~mask) */
+        TCGv res1 = tcg_temp_new();
+        tcg_gen_and_tl(res1, mask_neg, src);
+        /* calculate (1) | (base) */
+        TCGv res2 = tcg_temp_new();
+        tcg_gen_or_tl(res2, res1, base);
+        /* move result to dst */
+        tcg_gen_mov_i64(dst, res2);
+        /* free allocated temps */
+        tcg_temp_free(res1);
+        tcg_temp_free(res2);
+        tcg_temp_free_i64(mask_neg);
+        tcg_temp_free_i64(base);
+    }
+}
+
+static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
+{
+    gen_pm_adjust_address(s, addr, addr);
+    return addr;
+}
 /*
  * RISC-V requires NaN-boxing of narrower width floating point values.
  * This applies when a 32-bit value is assigned to a 64-bit FP register.
@@ -800,8 +836,36 @@  static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     } 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;
+            ctx->pm_base = env->mpmbase;
+            break;
+        default:
+            assert(0 && "Unreachable");
+        }
+    } else {
+        ctx->pm_enabled = 0;
+        ctx->pm_mask = 0;
+        ctx->pm_base = 0;
+    }
 #else
     ctx->virt_enabled = false;
+    ctx->pm_enabled = 0;
+    ctx->pm_mask = 0;
+    ctx->pm_base = 0;
 #endif
     ctx->misa = env->misa;
     ctx->frm = -1;  /* unknown rounding mode */
@@ -932,3 +996,4 @@  void riscv_translate_init(void)
     load_val = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, load_val),
                              "load_val");
 }
+