[PATCH v2 1/4] target/nios2: Shadow register set

Amir Gonnen posted 4 patches 3 years, 11 months ago
There is a newer version of this series
[PATCH v2 1/4] target/nios2: Shadow register set
Posted by Amir Gonnen 3 years, 11 months ago
Implement shadow register set and related instructions
rdprs, wrprs. Fix eret to update either status or sstatus
according to current register set.
eret also changes register set when needed.

Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>
---
 target/nios2/cpu.c       |  1 +
 target/nios2/cpu.h       | 47 ++++++++++++++++++++++++++++++++++++++--
 target/nios2/helper.h    |  3 +++
 target/nios2/op_helper.c | 24 ++++++++++++++++++++
 target/nios2/translate.c | 32 ++++++++++++++++++++++-----
 5 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 4cade61e93..0886705818 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -54,6 +54,7 @@ static void nios2_cpu_reset(DeviceState *dev)
     ncc->parent_reset(dev);

     memset(env->regs, 0, sizeof(uint32_t) * NUM_CORE_REGS);
+    memset(env->shadow_regs, 0, sizeof(uint32_t) * NUM_REG_SETS * NUM_GP_REGS);
     env->regs[R_PC] = cpu->reset_addr;

 #if defined(CONFIG_USER_ONLY)
diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index d2ba0c5bbd..1d3503825b 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -57,9 +57,14 @@ struct Nios2CPUClass {
 #define EXCEPTION_ADDRESS     0x00000004
 #define FAST_TLB_MISS_ADDRESS 0x00000008

+#define NUM_GP_REGS 32
+#define NUM_CR_REGS 32

 /* GP regs + CR regs + PC */
-#define NUM_CORE_REGS (32 + 32 + 1)
+#define NUM_CORE_REGS (NUM_GP_REGS + NUM_CR_REGS + 1)
+
+/* 63 shadow register sets. 0 is the primary set */
+#define NUM_REG_SETS 64

 /* General purpose register aliases */
 #define R_ZERO   0
@@ -80,7 +85,7 @@ struct Nios2CPUClass {
 #define R_RA     31

 /* Control register aliases */
-#define CR_BASE  32
+#define CR_BASE  NUM_GP_REGS
 #define CR_STATUS    (CR_BASE + 0)
 #define   CR_STATUS_PIE  (1 << 0)
 #define   CR_STATUS_U    (1 << 1)
@@ -88,7 +93,9 @@ struct Nios2CPUClass {
 #define   CR_STATUS_IH   (1 << 3)
 #define   CR_STATUS_IL   (63 << 4)
 #define   CR_STATUS_CRS  (63 << 10)
+#define   CR_STATUS_CRS_OFFSET 10
 #define   CR_STATUS_PRS  (63 << 16)
+#define   CR_STATUS_PRS_OFFSET 16
 #define   CR_STATUS_NMI  (1 << 22)
 #define   CR_STATUS_RSIE (1 << 23)
 #define CR_ESTATUS   (CR_BASE + 1)
@@ -131,6 +138,7 @@ struct Nios2CPUClass {

 /* Other registers */
 #define R_PC         64
+#define R_SSTATUS    30

 /* Exceptions */
 #define EXCP_BREAK    0x1000
@@ -157,6 +165,7 @@ struct Nios2CPUClass {

 struct CPUNios2State {
     uint32_t regs[NUM_CORE_REGS];
+    uint32_t shadow_regs[NUM_REG_SETS][NUM_GP_REGS];

 #if !defined(CONFIG_USER_ONLY)
     Nios2MMU mmu;
@@ -246,4 +255,38 @@ static inline void cpu_get_tb_cpu_state(CPUNios2State *env, target_ulong *pc,
     *flags = (env->regs[CR_STATUS] & (CR_STATUS_EH | CR_STATUS_U));
 }

+static inline uint32_t cpu_get_crs(const CPUNios2State *env)
+{
+    return (env->regs[CR_STATUS] & CR_STATUS_CRS)
+                    >> CR_STATUS_CRS_OFFSET;
+}
+
+static inline uint32_t cpu_get_prs(const CPUNios2State *env)
+{
+    return (env->regs[CR_STATUS] & CR_STATUS_PRS)
+                    >> CR_STATUS_PRS_OFFSET;
+}
+
+static inline void cpu_change_reg_set(CPUNios2State *env, uint32_t prev_set,
+                                      uint32_t new_set)
+{
+    if (new_set == prev_set) {
+        return;
+    }
+    memcpy(env->shadow_regs[prev_set], env->regs,
+           sizeof(uint32_t) * NUM_GP_REGS);
+    memcpy(env->regs, env->shadow_regs[new_set],
+           sizeof(uint32_t) * NUM_GP_REGS);
+    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & (~CR_STATUS_PRS))
+                       | ((prev_set << CR_STATUS_PRS_OFFSET) & CR_STATUS_PRS);
+    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & (~CR_STATUS_CRS))
+                       | ((new_set << CR_STATUS_CRS_OFFSET) & CR_STATUS_CRS);
+}
+
+static inline void cpu_set_crs(CPUNios2State *env, uint32_t value)
+{
+    uint32_t crs = cpu_get_crs(env);
+    cpu_change_reg_set(env, crs, value);
+}
+
 #endif /* NIOS2_CPU_H */
diff --git a/target/nios2/helper.h b/target/nios2/helper.h
index 6c8f0b5b35..3e5c016e9c 100644
--- a/target/nios2/helper.h
+++ b/target/nios2/helper.h
@@ -18,6 +18,9 @@
  * <http://www.gnu.org/licenses/lgpl-2.1.html>
  */

+DEF_HELPER_2(eret, void, env, i32)
+DEF_HELPER_3(wrprs, void, env, i32, i32)
+DEF_HELPER_2(rdprs, i32, env, i32)
 DEF_HELPER_FLAGS_2(raise_exception, TCG_CALL_NO_WG, noreturn, env, i32)

 #if !defined(CONFIG_USER_ONLY)
diff --git a/target/nios2/op_helper.c b/target/nios2/op_helper.c
index a59003855a..5e4f7a47ae 100644
--- a/target/nios2/op_helper.c
+++ b/target/nios2/op_helper.c
@@ -59,3 +59,27 @@ void helper_raise_exception(CPUNios2State *env, uint32_t index)
     cs->exception_index = index;
     cpu_loop_exit(cs);
 }
+
+void helper_wrprs(CPUNios2State *env, uint32_t reg_index, uint32_t value)
+{
+    uint32_t prs = cpu_get_prs(env);
+    env->shadow_regs[prs][reg_index] = value;
+}
+
+uint32_t helper_rdprs(CPUNios2State *env, uint32_t reg_index)
+{
+    uint32_t prs = cpu_get_prs(env);
+    return env->shadow_regs[prs][reg_index];
+}
+
+void helper_eret(CPUNios2State *env, uint32_t new_pc)
+{
+    uint32_t crs = cpu_get_crs(env);
+    if (crs == 0) {
+        env->regs[CR_STATUS] = env->regs[CR_ESTATUS];
+    } else {
+        env->regs[CR_STATUS] = env->regs[R_SSTATUS];
+    }
+    cpu_change_reg_set(env, crs, cpu_get_crs(env));
+    env->regs[R_PC] = new_pc;
+}
diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index f9abc2fdd2..e22ab1996a 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -205,6 +205,19 @@ static void call(DisasContext *dc, uint32_t code, uint32_t flags)
 /*
  * I-Type instructions
  */
+
+/*
+ * rB <- prs.rA + sigma(IMM16)
+ */
+static void rdprs(DisasContext *dc, uint32_t code, uint32_t flags)
+{
+    I_TYPE(instr, code);
+    TCGv t = tcg_temp_new();
+    gen_helper_rdprs(t, cpu_env, tcg_const_i32(instr.a));
+    tcg_gen_addi_tl(cpu_R[instr.b], t, instr.imm16.s);
+    tcg_temp_free(t);
+}
+
 /* Load instructions */
 static void gen_ldx(DisasContext *dc, uint32_t code, uint32_t flags)
 {
@@ -365,7 +378,7 @@ static const Nios2Instruction i_type_instructions[] = {
     INSTRUCTION_FLG(gen_stx, MO_SL),                  /* stwio */
     INSTRUCTION_FLG(gen_bxx, TCG_COND_LTU),           /* bltu */
     INSTRUCTION_FLG(gen_ldx, MO_UL),                  /* ldwio */
-    INSTRUCTION_UNIMPLEMENTED(),                      /* rdprs */
+    INSTRUCTION(rdprs),                               /* rdprs */
     INSTRUCTION_ILLEGAL(),
     INSTRUCTION_FLG(handle_r_type_instr, 0),          /* R-Type */
     INSTRUCTION_NOP(),                                /* flushd */
@@ -378,14 +391,23 @@ static const Nios2Instruction i_type_instructions[] = {
 /*
  * R-Type instructions
  */
+
+/*
+ * prs.rC <- rA
+ */
+static void wrprs(DisasContext *dc, uint32_t code, uint32_t flags)
+{
+    R_TYPE(instr, code);
+    gen_helper_wrprs(cpu_env, tcg_const_i32(instr.c), cpu_R[instr.a]);
+}
+
 /*
- * status <- estatus
+ * status <- CRS == 0? estatus: sstatus
  * PC <- ea
  */
 static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
-    tcg_gen_mov_tl(cpu_R[CR_STATUS], cpu_R[CR_ESTATUS]);
-    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_EA]);
+    gen_helper_eret(cpu_env, cpu_R[R_EA]);

     dc->base.is_jmp = DISAS_JUMP;
 }
@@ -672,7 +694,7 @@ static const Nios2Instruction r_type_instructions[] = {
     INSTRUCTION_ILLEGAL(),
     INSTRUCTION(slli),                                /* slli */
     INSTRUCTION(sll),                                 /* sll */
-    INSTRUCTION_UNIMPLEMENTED(),                      /* wrprs */
+    INSTRUCTION(wrprs),                               /* wrprs */
     INSTRUCTION_ILLEGAL(),
     INSTRUCTION(or),                                  /* or */
     INSTRUCTION(mulxsu),                              /* mulxsu */
--
2.25.1


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

Re: [PATCH v2 1/4] target/nios2: Shadow register set
Posted by Richard Henderson 3 years, 11 months ago
On 2/24/22 03:48, Amir Gonnen wrote:
> @@ -88,7 +93,9 @@ struct Nios2CPUClass {
>   #define   CR_STATUS_IH   (1 << 3)
>   #define   CR_STATUS_IL   (63 << 4)
>   #define   CR_STATUS_CRS  (63 << 10)
> +#define   CR_STATUS_CRS_OFFSET 10
>   #define   CR_STATUS_PRS  (63 << 16)
> +#define   CR_STATUS_PRS_OFFSET 16
>   #define   CR_STATUS_NMI  (1 << 22)
>   #define   CR_STATUS_RSIE (1 << 23)
>   #define CR_ESTATUS   (CR_BASE + 1)

It would be preferable to use hw/registerfields.h:

FIELD(CR_STATUS, IL, 4, 6)
FIELD(CR_STATUS, CRS, 10, 6)
FIELD(CR_STATUS, PRS, 16, 6)

> +static inline uint32_t cpu_get_crs(const CPUNios2State *env)
> +{
> +    return (env->regs[CR_STATUS] & CR_STATUS_CRS)
> +                    >> CR_STATUS_CRS_OFFSET;
> +}

This becomes

     return FIELD_EX32(env->regs[CR_STATUS], CR_STATUS, CRS);

> +    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & (~CR_STATUS_PRS))
> +                       | ((prev_set << CR_STATUS_PRS_OFFSET) & CR_STATUS_PRS);

This becomes

     env->regs[CR_STATUS] = FIELD_DP32(env->regs[CR_STATUS],
                                       CR_STATUS, PRS, prev_set);

etc.


r~

Re: [PATCH v2 1/4] target/nios2: Shadow register set
Posted by Richard Henderson 3 years, 11 months ago
On 2/24/22 03:48, Amir Gonnen wrote:
> +void helper_eret(CPUNios2State *env, uint32_t new_pc)
> +{
> +    uint32_t crs = cpu_get_crs(env);
> +    if (crs == 0) {
> +        env->regs[CR_STATUS] = env->regs[CR_ESTATUS];
> +    } else {
> +        env->regs[CR_STATUS] = env->regs[R_SSTATUS];
> +    }
> +    cpu_change_reg_set(env, crs, cpu_get_crs(env));

Hmm.  This could probably use a comment that the second computation of cpu_get_crs is 
using the value just restored into CR_STATUS.


> +void helper_wrprs(CPUNios2State *env, uint32_t reg_index, uint32_t value)
> +{
> +    uint32_t prs = cpu_get_prs(env);
> +    env->shadow_regs[prs][reg_index] = value;
> +}
> +
> +uint32_t helper_rdprs(CPUNios2State *env, uint32_t reg_index)
> +{
> +    uint32_t prs = cpu_get_prs(env);
> +    return env->shadow_regs[prs][reg_index];
> +}

These are fairly easy to compute inline, e.g. for rdprs:

     TCGv_i32 t = tcg_temp_i32_new();
     TCGv_ptr p = tcg_temp_ptr_new();

     tcg_gen_extract_i32(t, cpu_R[CR_STATUS],
                         R_CR_STATUS_CRS_SHIFT,
                         R_CR_STATUS_CRS_LENGTH);
     tcg_gen_muli_i32(t, t, sizeof(uint32_t) * NUM_GP_REGS);
     tcg_gen_ext_i32_ptr(p, t);

     tcg_gen_add_ptr(p, p, cpu_env);
     tcg_gen_ld_i32(t, p, offsetof(CPUNios2State, shadow_regs)
                     + sizeof(uint32_t) * instr.a);
     tcg_gen_addi_i32(cpu_R[instr.b], t, instr.imm16.s);

     tcg_temp_free_ptr(p);
     tcg_temp_free_i32(o);

> +static void rdprs(DisasContext *dc, uint32_t code, uint32_t flags)
> +{
> +    I_TYPE(instr, code);
> +    TCGv t = tcg_temp_new();
> +    gen_helper_rdprs(t, cpu_env, tcg_const_i32(instr.a));

You're missing a gen_check_supervisor here and in wrprs.

>  static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
>  {
> -    tcg_gen_mov_tl(cpu_R[CR_STATUS], cpu_R[CR_ESTATUS]);
> -    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_EA]);
> +    gen_helper_eret(cpu_env, cpu_R[R_EA]);

As an existing bug to be fixed by a separate patch, eret should also check for supervisor.

> 
> The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.
> 

You really need to suppress these footers when posting to a public mailing list.


r~

RE: [PATCH v2 1/4] target/nios2: Shadow register set
Posted by Amir Gonnen 3 years, 11 months ago
Hi Richard,

Thank you for your review and comments!

> You're missing a gen_check_supervisor here and in wrprs.

There's something I don't understand about gen_check_supervisor - it looks like it checks CR_STATUS_U when generating code instead of generating code that checks CR_STATUS_U.
Is that correct to assume that CR_STATUS_U would remain the same between generation and runtime?

> As an existing bug to be fixed by a separate patch, eret should also check for supervisor.

Do you suggest I shouldn't fix this here? Why not fix this anyway?

> You really need to suppress these footers when posting to a public mailing list.

I'm sorry about that.
I've worked with IT team to disable this automatic footer when sending to the mailing list so it shouldn't appear any more in the list, but it may still appear for individual recipients.
So, if this still annoys anyone please let me know and I'll contact them again for a different solution.
Re: [PATCH v2 1/4] target/nios2: Shadow register set
Posted by Peter Maydell 3 years, 11 months ago
On Sun, 27 Feb 2022 at 16:16, Amir Gonnen <amir.gonnen@neuroblade.ai> wrote:
> There's something I don't understand about gen_check_supervisor -
> it looks like it checks CR_STATUS_U when generating code instead
> of generating code that checks CR_STATUS_U.

This is OK because it is checking the value of CR_STATUS_U in
the tbflags, not the one in the CPU state. Basically, when QEMU
looks for a pre-existing TB it does so by looking up in a hash
table where the key is (program counter, flags, cs_base). (cs_base
here is named what it is for historical reasons, but it can be
used for any data the target likes.) So the target code can
put some parts of its CPU state into the TB flags word, and then
at code-generation time it can generate code that assumes the
value of that state. If we ever run the same bit of code both in
supervisor and non-supervisor mode, we generate two separate
TBs for it. (You can see what nios2 is putting in the flags if
you look at cpu_get_tb_cpu_state() in cpu.h -- currently it
just keeps the U and EH status bits there.)

> > As an existing bug to be fixed by a separate patch, eret should also check for supervisor.
>
> Do you suggest I shouldn't fix this here? Why not fix this anyway?

It should go in a separate patch (but you can put that patch in
a v3 of this series), because it's a separate bug
fix -- it is not related to support of shadow registers.
We like to keep distinct changes in distinct patches (and thus
git commits, because it makes things easier to code review and
also means we have more information if we need to track down
the reason for a change or diagnose a regression in future.

thanks
-- PMM