Re: [PATCH for-7.1 v6 41/51] target/nios2: Introduce shadow register sets

[Peter Maydell]

On Thu, 17 Mar 2022 at 05:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Do not actually enable them so far, but add all of the
> plumbing to address them.  Do not enable them for user-only.
>
> Add an env->regs pointer that handles the indirection to
> the current register set.  The naming of the pointer hides
> the difference between old and new, user-only and sysemu.
>
> From the notes on wrprs, which states that r0 must be initialized
> before use in shadow register sets, infer that R_ZERO is *not*
> hardwired to zero in shadow register sets.  Adjust load_gpr and
> dest_gpr to reflect this.  At the same time we might as well
> special case crs == 0 to avoid the indirection through env->regs
> during translation as well.  Given that this is intended to be
> the most common case for non-interrupt handlers.

>  static TCGv load_gpr(DisasContext *dc, unsigned reg)
>  {
>      assert(reg < NUM_GP_REGS);
> -    if (unlikely(reg == R_ZERO)) {
> -        return tcg_constant_tl(0);
> +    if (dc->crs0) {
> +        if (unlikely(reg == R_ZERO)) {
> +            return tcg_constant_tl(0);
> +        }
> +        return cpu_R[reg];
>      }
> -    return cpu_R[reg];
> +#ifdef CONFIG_USER_ONLY
> +    g_assert_not_reached();
> +#else
> +    return cpu_crs_R[reg];
> +#endif
>  }
>
>  static TCGv dest_gpr(DisasContext *dc, unsigned reg)
>  {
>      assert(reg < NUM_GP_REGS);
> -    if (unlikely(reg == R_ZERO)) {
> -        if (dc->sink == NULL) {
> -            dc->sink = tcg_temp_new();
> +    if (dc->crs0) {
> +        if (unlikely(reg == R_ZERO)) {
> +            if (dc->sink == NULL) {
> +                dc->sink = tcg_temp_new();
> +            }
> +            return dc->sink;
>          }
> -        return dc->sink;
> +        return cpu_R[reg];
>      }
> -    return cpu_R[reg];
> +#ifdef CONFIG_USER_ONLY
> +    g_assert_not_reached();
> +#else
> +    return cpu_crs_R[reg];
> +#endif
>  }

The behaviour of r0 in the shadow register sets is definitely
underspecified, but I really don't believe that r0 is a normal
writeable register for everything except the crs=0 set, which
is what you've implemented here. My best guess is:
 * registers are implemented as a pile of RAM, including r0
 * on reset the set-0 r0 is reset to 0, but nothing else is
   (this bit's actually in the spec)
 * writes to r0 are always discarded, except for the special
   case of wrprs

I'm tempted to suggest we should make our tbflags bit
"we know r0 is zero" -- the guest doesn't have many ways
to switch register set, basically I think just eret and taking
an external interrupt, and those either happen outside the
TB or are going to end the TB anyway. Can we make
cpu_get_tb_cpu_state() simply set the TB flag if
 env->shadow_regs[crs][0] == 0
or have I missed something that means that won't work?

(I actually wouldn't care to bet much money on wrprs being
unable to write to register-set-0 r0. It would be interesting
to test that on the real hardware.)

thanks
-- PMM