Re: [PATCH v3 2/5] target/nios2: Shadow register set

[Richard Henderson]

On 3/3/22 05:39, Amir Gonnen wrote:
> 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       | 48 +++++++++++++++++++++++++++---
>   target/nios2/helper.h    |  1 +
>   target/nios2/op_helper.c | 18 +++++++++++
>   target/nios2/translate.c | 64 ++++++++++++++++++++++++++++++++++++----
>   5 files changed, 123 insertions(+), 9 deletions(-)
>
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index 6975ae4bdb..026ee18b01 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);

Zeroing registers is not part of processing 3.7.4 Reset Exception.
I'd be tempted to set all of the registers to 0xdeadbeef instead, to catch out incorrect 
code, especially wrt the shadowed r0.

> -#define   CR_STATUS_CRS  (63 << 10)
> -#define   CR_STATUS_PRS  (63 << 16)
> +FIELD(CR_STATUS, CRS, 10, 6)
> +FIELD(CR_STATUS, PRS, 16, 6)

This change needs to be done separately from introducing shadow registers.

Having read the specification more closely now, I think this implementation may be wrong. 
 In particular:

(1) r0 appears to be hardwired to 0 only in the normal register set (CRS = 0).  That's why 
software is directed to use wrprs to initialize r0 in each shadow register set to 0.

(2) Changes are needed to wrctl to protect the read-only bits of the control registers. 
In this case, especially status.NMI and status.CRS.

(3) The advice I gave you for rdprs/wrprs is wrong when CRS == PRS (you'd need to modify 
regs[n] not shadow_regs[prs][n]).

These 3 items need to be handled in separate patches.

> +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] =
> +        FIELD_DP32(env->regs[CR_STATUS], CR_STATUS, PRS, prev_set);
> +    env->regs[CR_STATUS] =
> +        FIELD_DP32(env->regs[CR_STATUS], CR_STATUS, CRS, new_set);
> +}

Another possibility, that doesn't involve moving data around is to use a pointer to the 
CRS base, like sparc does for its register windows.  I'm not necessarily advocating this 
solution, merely pointing it out.


r~