[PATCH v2 0/4] linux/sparc: more get/set_context fixes

[PATCH v2 0/4] linux/sparc: more get/set_context fixes

[Peter Maydell]

Based-on: 20201105212314.9628-1-peter.maydell@linaro.org
("[PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs")

This series fixes a few more issues with our sparc linux-user
sparc64_get_context() and sparc64_set_context() implementation:
 * we weren't handling FPU regs correctly, and also the way
   we coded the handling triggered Coverity warnings
 * some stray pointless error checks
 * we shouldn't restore %g7 in set_context
 * we weren't saving and restoring tstate correctly

My main aim here was to deal with the Coverity errors, but
the rest are things I noticed while I was working on the
code or which had fixme comments, and I figured I'd fix
them while the code was fresh in my mind.

thanks
-- PMM

Peter Maydell (4):
  linux-user/sparc: Correct sparc64_get/set_context() FPU handling
  linux-user/sparc: Remove unneeded checks of 'err' from
    sparc64_get_context()
  linux-user/sparc: Don't restore %g7 in sparc64_set_context()
  linux-user/sparc: Handle tstate in sparc64_get/set_context()

 target/sparc/cpu.h          | 28 +++++++++---
 linux-user/sparc/signal.c   | 87 ++++++++++++++++++++-----------------
 target/sparc/int64_helper.c |  5 +--
 3 files changed, 71 insertions(+), 49 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling

[Peter Maydell]

The handling of the FPU state in sparc64_get_context() and
sparc64_set_context() is not the same as what the kernel actually
does: we unconditionally read and write the FP registers and the
FSR, GSR and FPRS, but the kernel logic is more complicated:
 * in get_context the kernel has code for saving FPU registers,
   but it is hidden inside an "if (fenab) condition and the
   fenab flag is always set to 0 (inside an "#if 1" which has
   been in the kernel for over 15 years). So the effect is that
   the FPU state part is always written as zeroes.
 * in set_context the kernel looks at the fenab field in the
   structure from the guest, and only restores the state if
   it is set; it also looks at the structure's FPRS to see
   whether either the upper or lower or both halves of the
   register file have valid data.

Bring our implementations into line with the kernel:
 * in get_context:
    - clear the entire target_ucontext at the top of the
      function (as the kernel does)
    - then don't write the FPU state, so those fields remain zero
    - this fixes Coverity issue CID 1432305 by deleting the code
      it was complaining about
 * in set_context:
    - check the fenab and the fpsr to decide which parts of
      the FPU data to restore, if any
    - instead of setting the FPU registers by doing two
      32-bit loads and filling in the .upper and .lower parts
      of the CPU_Double union separately, just do a 64-bit
      load of the whole register at once. This fixes Coverity
      issue CID 1432303 because we now access the dregs[] part
      of the mcfpu_fregs union rather than the sregs[] part
      (which is not large enough to actually cover the whole of
      the data, so we were accessing off the end of sregs[])

We change both functions in a single commit to avoid potentially
breaking bisection.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/cpu.h        |  4 ++-
 linux-user/sparc/signal.c | 74 +++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index b9369398f24..277254732b9 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -156,7 +156,9 @@ enum {
 #define PS_IE    (1<<1)
 #define PS_AG    (1<<0) /* v9, zero on UA2007 */
 
-#define FPRS_FEF (1<<2)
+#define FPRS_DL (1 << 0)
+#define FPRS_DU (1 << 1)
+#define FPRS_FEF (1 << 2)
 
 #define HS_PRIV  (1<<2)
 #endif
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index d12adc8e6ff..e661a769cb1 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -402,8 +402,10 @@ void sparc64_set_context(CPUSPARCState *env)
     abi_ulong ucp_addr;
     struct target_ucontext *ucp;
     target_mc_gregset_t *grp;
+    target_mc_fpu_t *fpup;
     abi_ulong pc, npc, tstate;
     unsigned int i;
+    unsigned char fenab;
 
     ucp_addr = env->regwptr[WREG_O0];
     if (!lock_user_struct(VERIFY_READ, ucp, ucp_addr, 1)) {
@@ -467,26 +469,42 @@ void sparc64_set_context(CPUSPARCState *env)
     __get_user(env->regwptr[WREG_FP], &(ucp->tuc_mcontext.mc_fp));
     __get_user(env->regwptr[WREG_I7], &(ucp->tuc_mcontext.mc_i7));
 
-    /* FIXME this does not match how the kernel handles the FPU in
-     * its sparc64_set_context implementation. In particular the FPU
-     * is only restored if fenab is non-zero in:
-     *   __get_user(fenab, &(ucp->tuc_mcontext.mc_fpregs.mcfpu_enab));
-     */
-    __get_user(env->fprs, &(ucp->tuc_mcontext.mc_fpregs.mcfpu_fprs));
-    {
-        uint32_t *src = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs;
-        for (i = 0; i < 64; i++, src++) {
-            if (i & 1) {
-                __get_user(env->fpr[i/2].l.lower, src);
-            } else {
-                __get_user(env->fpr[i/2].l.upper, src);
+    fpup = &ucp->tuc_mcontext.mc_fpregs;
+
+    __get_user(fenab, &(fpup->mcfpu_enab));
+    if (fenab) {
+        abi_ulong fprs;
+
+        /*
+         * We use the FPRS from the guest only in deciding whether
+         * to restore the upper, lower, or both banks of the FPU regs.
+         * The kernel here writes the FPU register data into the
+         * process's current_thread_info state and unconditionally
+         * clears FPRS and TSTATE_PEF: this disables the FPU so that the
+         * next FPU-disabled trap will copy the data out of
+         * current_thread_info and into the real FPU registers.
+         * QEMU doesn't need to handle lazy-FPU-state-restoring like that,
+         * so we always load the data directly into the FPU registers
+         * and leave FPRS and TSTATE_PEF alone (so the FPU stays enabled).
+         * Note that because we (and the kernel) always write zeroes for
+         * the fenab and fprs in sparc64_get_context() none of this code
+         * will execute unless the guest manually constructed or changed
+         * the context structure.
+         */
+        __get_user(fprs, &(fpup->mcfpu_fprs));
+        if (fprs & FPRS_DL) {
+            for (i = 0; i < 16; i++) {
+                __get_user(env->fpr[i].ll, &(fpup->mcfpu_fregs.dregs[i]));
             }
         }
+        if (fprs & FPRS_DU) {
+            for (i = 16; i < 31; i++) {
+                __get_user(env->fpr[i].ll, &(fpup->mcfpu_fregs.dregs[i]));
+            }
+        }
+        __get_user(env->fsr, &(fpup->mcfpu_fsr));
+        __get_user(env->gsr, &(fpup->mcfpu_gsr));
     }
-    __get_user(env->fsr,
-               &(ucp->tuc_mcontext.mc_fpregs.mcfpu_fsr));
-    __get_user(env->gsr,
-               &(ucp->tuc_mcontext.mc_fpregs.mcfpu_gsr));
     unlock_user_struct(ucp, ucp_addr, 0);
     return;
 do_sigsegv:
@@ -509,7 +527,9 @@ void sparc64_get_context(CPUSPARCState *env)
     if (!lock_user_struct(VERIFY_WRITE, ucp, ucp_addr, 0)) {
         goto do_sigsegv;
     }
-    
+
+    memset(ucp, 0, sizeof(*ucp));
+
     mcp = &ucp->tuc_mcontext;
     grp = &mcp->mc_gregs;
 
@@ -572,19 +592,11 @@ void sparc64_get_context(CPUSPARCState *env)
     __put_user(env->regwptr[WREG_FP], &(mcp->mc_fp));
     __put_user(env->regwptr[WREG_I7], &(mcp->mc_i7));
 
-    {
-        uint32_t *dst = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs;
-        for (i = 0; i < 64; i++, dst++) {
-            if (i & 1) {
-                __put_user(env->fpr[i/2].l.lower, dst);
-            } else {
-                __put_user(env->fpr[i/2].l.upper, dst);
-            }
-        }
-    }
-    __put_user(env->fsr, &(mcp->mc_fpregs.mcfpu_fsr));
-    __put_user(env->gsr, &(mcp->mc_fpregs.mcfpu_gsr));
-    __put_user(env->fprs, &(mcp->mc_fpregs.mcfpu_fprs));
+    /*
+     * We don't write out the FPU state. This matches the kernel's
+     * implementation (which has the code for doing this but
+     * hidden behind an "if (fenab)" where fenab is always 0).
+     */
 
     if (err)
         goto do_sigsegv;
-- 
2.20.1

Re: [PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling

[Richard Henderson]

On 11/6/20 7:27 AM, Peter Maydell wrote:
> +        if (fprs & FPRS_DU) {
> +            for (i = 16; i < 31; i++) {

32.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling

[Peter Maydell]

On Fri, 6 Nov 2020 at 17:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/6/20 7:27 AM, Peter Maydell wrote:
> > +        if (fprs & FPRS_DU) {
> > +            for (i = 16; i < 31; i++) {
>
> 32.

Derp. Lucky this code basically never gets run, eh ? :-)

-- PMM

Re: [PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling

[Laurent Vivier]

Le 06/11/2020 à 18:09, Richard Henderson a écrit :
> On 11/6/20 7:27 AM, Peter Maydell wrote:
>> +        if (fprs & FPRS_DU) {
>> +            for (i = 16; i < 31; i++) {
> 
> 32.
> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> r~
> 

Applied to my linux-user-for-6.0 branch with this change.

Thanks,
Laurent

[PATCH v2 2/4] linux-user/sparc: Remove unneeded checks of 'err' from sparc64_get_context()

[Peter Maydell]

Unlike the kernel macros, our __get_user() and __put_user() do not
return a failure code.  Kernel code typically has a style of
  err |= __get_user(...); err |= __get_user(...);
and then checking err at the end.  In sparc64_get_context() our
version of the code dropped the accumulating into err but left the
"if (err) goto do_sigsegv" checks, which will never be taken. Delete
unnecessary if()s.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/sparc/signal.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index e661a769cb1..43dcd137f51 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -555,8 +555,6 @@ void sparc64_get_context(CPUSPARCState *env)
         for (i = 0; i < TARGET_NSIG_WORDS; i++, dst++, src++) {
             __put_user(*src, dst);
         }
-        if (err)
-            goto do_sigsegv;
     }
 
     /* XXX: tstate must be saved properly */
@@ -598,8 +596,6 @@ void sparc64_get_context(CPUSPARCState *env)
      * hidden behind an "if (fenab)" where fenab is always 0).
      */
 
-    if (err)
-        goto do_sigsegv;
     unlock_user_struct(ucp, ucp_addr, 1);
     return;
 do_sigsegv:
-- 
2.20.1

Re: [PATCH v2 2/4] linux-user/sparc: Remove unneeded checks of 'err' from sparc64_get_context()

[Richard Henderson]

On 11/6/20 7:27 AM, Peter Maydell wrote:
> Unlike the kernel macros, our __get_user() and __put_user() do not
> return a failure code.  Kernel code typically has a style of
>   err |= __get_user(...); err |= __get_user(...);
> and then checking err at the end.  In sparc64_get_context() our
> version of the code dropped the accumulating into err but left the
> "if (err) goto do_sigsegv" checks, which will never be taken. Delete
> unnecessary if()s.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 2/4] linux-user/sparc: Remove unneeded checks of 'err' from sparc64_get_context()

[Laurent Vivier]

Le 06/11/2020 à 16:27, Peter Maydell a écrit :
> Unlike the kernel macros, our __get_user() and __put_user() do not
> return a failure code.  Kernel code typically has a style of
>   err |= __get_user(...); err |= __get_user(...);
> and then checking err at the end.  In sparc64_get_context() our
> version of the code dropped the accumulating into err but left the
> "if (err) goto do_sigsegv" checks, which will never be taken. Delete
> unnecessary if()s.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
> index e661a769cb1..43dcd137f51 100644
> --- a/linux-user/sparc/signal.c
> +++ b/linux-user/sparc/signal.c
> @@ -555,8 +555,6 @@ void sparc64_get_context(CPUSPARCState *env)
>          for (i = 0; i < TARGET_NSIG_WORDS; i++, dst++, src++) {
>              __put_user(*src, dst);
>          }
> -        if (err)
> -            goto do_sigsegv;
>      }
>  
>      /* XXX: tstate must be saved properly */
> @@ -598,8 +596,6 @@ void sparc64_get_context(CPUSPARCState *env)
>       * hidden behind an "if (fenab)" where fenab is always 0).
>       */
>  
> -    if (err)
> -        goto do_sigsegv;
>      unlock_user_struct(ucp, ucp_addr, 1);
>      return;
>  do_sigsegv:
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent

[PATCH v2 3/4] linux-user/sparc: Don't restore %g7 in sparc64_set_context()

[Peter Maydell]

The kernel does not restore the g7 register in sparc64_set_context();
neither should we. (We still save it in sparc64_get_context().)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/sparc/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index 43dcd137f51..ed32c7abd17 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -447,7 +447,7 @@ void sparc64_set_context(CPUSPARCState *env)
     __get_user(env->gregs[4], (&(*grp)[SPARC_MC_G4]));
     __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5]));
     __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6]));
-    __get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7]));
+    /* Skip g7 as that's the thread register in userspace */
 
     /*
      * Note that unlike the kernel, we didn't need to mess with the
-- 
2.20.1

Re: [PATCH v2 3/4] linux-user/sparc: Don't restore %g7 in sparc64_set_context()

[Richard Henderson]

On 11/6/20 7:27 AM, Peter Maydell wrote:
> The kernel does not restore the g7 register in sparc64_set_context();
> neither should we. (We still save it in sparc64_get_context().)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 3/4] linux-user/sparc: Don't restore %g7 in sparc64_set_context()

[Laurent Vivier]

Le 06/11/2020 à 16:27, Peter Maydell a écrit :
> The kernel does not restore the g7 register in sparc64_set_context();
> neither should we. (We still save it in sparc64_get_context().)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
> index 43dcd137f51..ed32c7abd17 100644
> --- a/linux-user/sparc/signal.c
> +++ b/linux-user/sparc/signal.c
> @@ -447,7 +447,7 @@ void sparc64_set_context(CPUSPARCState *env)
>      __get_user(env->gregs[4], (&(*grp)[SPARC_MC_G4]));
>      __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5]));
>      __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6]));
> -    __get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7]));
> +    /* Skip g7 as that's the thread register in userspace */
>  
>      /*
>       * Note that unlike the kernel, we didn't need to mess with the
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent

[PATCH v2 4/4] linux-user/sparc: Handle tstate in sparc64_get/set_context()

[Peter Maydell]

Correctly implement save/restore of the tstate field in
sparc64_get_context() and sparc64_set_context():
 * Don't use the CWP value from the guest in set_context
 * Construct and save a tstate value rather than leaving
   it as zero in get_context

To do this we factor out the "calculate TSTATE value from CPU state"
code from sparc_cpu_do_interrupt() into its own sparc64_tstate()
function; that in turn requires us to move some of the function
prototypes out from inside a CPU_NO_IO_DEFS ifdef guard.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/cpu.h          | 24 ++++++++++++++++++++----
 linux-user/sparc/signal.c   |  7 +++----
 target/sparc/int64_helper.c |  5 +----
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 277254732b9..4b2290650be 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -608,10 +608,6 @@ target_ulong cpu_get_psr(CPUSPARCState *env1);
 void cpu_put_psr(CPUSPARCState *env1, target_ulong val);
 void cpu_put_psr_raw(CPUSPARCState *env1, target_ulong val);
 #ifdef TARGET_SPARC64
-target_ulong cpu_get_ccr(CPUSPARCState *env1);
-void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
-target_ulong cpu_get_cwp64(CPUSPARCState *env1);
-void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
 void cpu_change_pstate(CPUSPARCState *env1, uint32_t new_pstate);
 void cpu_gl_switch_gregs(CPUSPARCState *env, uint32_t new_gl);
 #endif
@@ -829,4 +825,24 @@ static inline bool tb_am_enabled(int tb_flags)
 #endif
 }
 
+#ifdef TARGET_SPARC64
+/* win_helper.c */
+target_ulong cpu_get_ccr(CPUSPARCState *env1);
+void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
+target_ulong cpu_get_cwp64(CPUSPARCState *env1);
+void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
+
+static inline uint64_t sparc64_tstate(CPUSPARCState *env)
+{
+    uint64_t tstate = (cpu_get_ccr(env) << 32) |
+        ((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
+        cpu_get_cwp64(env);
+
+    if (env->def.features & CPU_FEATURE_GL) {
+        tstate |= (env->gl & 7ULL) << 40;
+    }
+    return tstate;
+}
+#endif
+
 #endif
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index ed32c7abd17..a6c7c7664a2 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -438,9 +438,9 @@ void sparc64_set_context(CPUSPARCState *env)
     env->npc = npc;
     __get_user(env->y, &((*grp)[SPARC_MC_Y]));
     __get_user(tstate, &((*grp)[SPARC_MC_TSTATE]));
+    /* Honour TSTATE_ASI, TSTATE_ICC and TSTATE_XCC only */
     env->asi = (tstate >> 24) & 0xff;
-    cpu_put_ccr(env, tstate >> 32);
-    cpu_put_cwp64(env, tstate & 0x1f);
+    cpu_put_ccr(env, (tstate >> 32) & 0xff);
     __get_user(env->gregs[1], (&(*grp)[SPARC_MC_G1]));
     __get_user(env->gregs[2], (&(*grp)[SPARC_MC_G2]));
     __get_user(env->gregs[3], (&(*grp)[SPARC_MC_G3]));
@@ -557,8 +557,7 @@ void sparc64_get_context(CPUSPARCState *env)
         }
     }
 
-    /* XXX: tstate must be saved properly */
-    //    __put_user(env->tstate, &((*grp)[SPARC_MC_TSTATE]));
+    __put_user(sparc64_tstate(env), &((*grp)[SPARC_MC_TSTATE]));
     __put_user(env->pc, &((*grp)[SPARC_MC_PC]));
     __put_user(env->npc, &((*grp)[SPARC_MC_NPC]));
     __put_user(env->y, &((*grp)[SPARC_MC_Y]));
diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
index f3e7f32de61..735668f5006 100644
--- a/target/sparc/int64_helper.c
+++ b/target/sparc/int64_helper.c
@@ -131,9 +131,7 @@ void sparc_cpu_do_interrupt(CPUState *cs)
     }
     tsptr = cpu_tsptr(env);
 
-    tsptr->tstate = (cpu_get_ccr(env) << 32) |
-        ((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
-        cpu_get_cwp64(env);
+    tsptr->tstate = sparc64_tstate(env);
     tsptr->tpc = env->pc;
     tsptr->tnpc = env->npc;
     tsptr->tt = intno;
@@ -148,7 +146,6 @@ void sparc_cpu_do_interrupt(CPUState *cs)
     }
 
     if (env->def.features & CPU_FEATURE_GL) {
-        tsptr->tstate |= (env->gl & 7ULL) << 40;
         cpu_gl_switch_gregs(env, env->gl + 1);
         env->gl++;
     }
-- 
2.20.1

Re: [PATCH v2 4/4] linux-user/sparc: Handle tstate in sparc64_get/set_context()

[Richard Henderson]

On 11/6/20 7:27 AM, Peter Maydell wrote:
> +#ifdef TARGET_SPARC64
> +/* win_helper.c */
> +target_ulong cpu_get_ccr(CPUSPARCState *env1);
> +void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
> +target_ulong cpu_get_cwp64(CPUSPARCState *env1);
> +void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
> +
> +static inline uint64_t sparc64_tstate(CPUSPARCState *env)
> +{
> +    uint64_t tstate = (cpu_get_ccr(env) << 32) |
> +        ((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
> +        cpu_get_cwp64(env);
> +
> +    if (env->def.features & CPU_FEATURE_GL) {
> +        tstate |= (env->gl & 7ULL) << 40;
> +    }
> +    return tstate;
> +}
> +#endif

Given that this inline function calls 2 other out-of-line functions, I think it
might as well be out-of-line itself.
I'd place it in win_helper.c alongside the functions that it calls.

But either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/4] linux-user/sparc: Handle tstate in sparc64_get/set_context()

[Laurent Vivier]

Le 06/11/2020 à 16:27, Peter Maydell a écrit :
> Correctly implement save/restore of the tstate field in
> sparc64_get_context() and sparc64_set_context():
>  * Don't use the CWP value from the guest in set_context
>  * Construct and save a tstate value rather than leaving
>    it as zero in get_context
> 
> To do this we factor out the "calculate TSTATE value from CPU state"
> code from sparc_cpu_do_interrupt() into its own sparc64_tstate()
> function; that in turn requires us to move some of the function
> prototypes out from inside a CPU_NO_IO_DEFS ifdef guard.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/cpu.h          | 24 ++++++++++++++++++++----
>  linux-user/sparc/signal.c   |  7 +++----
>  target/sparc/int64_helper.c |  5 +----
>  3 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> index 277254732b9..4b2290650be 100644
> --- a/target/sparc/cpu.h
> +++ b/target/sparc/cpu.h
> @@ -608,10 +608,6 @@ target_ulong cpu_get_psr(CPUSPARCState *env1);
>  void cpu_put_psr(CPUSPARCState *env1, target_ulong val);
>  void cpu_put_psr_raw(CPUSPARCState *env1, target_ulong val);
>  #ifdef TARGET_SPARC64
> -target_ulong cpu_get_ccr(CPUSPARCState *env1);
> -void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
> -target_ulong cpu_get_cwp64(CPUSPARCState *env1);
> -void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
>  void cpu_change_pstate(CPUSPARCState *env1, uint32_t new_pstate);
>  void cpu_gl_switch_gregs(CPUSPARCState *env, uint32_t new_gl);
>  #endif
> @@ -829,4 +825,24 @@ static inline bool tb_am_enabled(int tb_flags)
>  #endif
>  }
>  
> +#ifdef TARGET_SPARC64
> +/* win_helper.c */
> +target_ulong cpu_get_ccr(CPUSPARCState *env1);
> +void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
> +target_ulong cpu_get_cwp64(CPUSPARCState *env1);
> +void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
> +
> +static inline uint64_t sparc64_tstate(CPUSPARCState *env)
> +{
> +    uint64_t tstate = (cpu_get_ccr(env) << 32) |
> +        ((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
> +        cpu_get_cwp64(env);
> +
> +    if (env->def.features & CPU_FEATURE_GL) {
> +        tstate |= (env->gl & 7ULL) << 40;
> +    }
> +    return tstate;
> +}
> +#endif
> +
>  #endif
> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
> index ed32c7abd17..a6c7c7664a2 100644
> --- a/linux-user/sparc/signal.c
> +++ b/linux-user/sparc/signal.c
> @@ -438,9 +438,9 @@ void sparc64_set_context(CPUSPARCState *env)
>      env->npc = npc;
>      __get_user(env->y, &((*grp)[SPARC_MC_Y]));
>      __get_user(tstate, &((*grp)[SPARC_MC_TSTATE]));
> +    /* Honour TSTATE_ASI, TSTATE_ICC and TSTATE_XCC only */
>      env->asi = (tstate >> 24) & 0xff;
> -    cpu_put_ccr(env, tstate >> 32);
> -    cpu_put_cwp64(env, tstate & 0x1f);
> +    cpu_put_ccr(env, (tstate >> 32) & 0xff);
>      __get_user(env->gregs[1], (&(*grp)[SPARC_MC_G1]));
>      __get_user(env->gregs[2], (&(*grp)[SPARC_MC_G2]));
>      __get_user(env->gregs[3], (&(*grp)[SPARC_MC_G3]));
> @@ -557,8 +557,7 @@ void sparc64_get_context(CPUSPARCState *env)
>          }
>      }
>  
> -    /* XXX: tstate must be saved properly */
> -    //    __put_user(env->tstate, &((*grp)[SPARC_MC_TSTATE]));
> +    __put_user(sparc64_tstate(env), &((*grp)[SPARC_MC_TSTATE]));
>      __put_user(env->pc, &((*grp)[SPARC_MC_PC]));
>      __put_user(env->npc, &((*grp)[SPARC_MC_NPC]));
>      __put_user(env->y, &((*grp)[SPARC_MC_Y]));
> diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
> index f3e7f32de61..735668f5006 100644
> --- a/target/sparc/int64_helper.c
> +++ b/target/sparc/int64_helper.c
> @@ -131,9 +131,7 @@ void sparc_cpu_do_interrupt(CPUState *cs)
>      }
>      tsptr = cpu_tsptr(env);
>  
> -    tsptr->tstate = (cpu_get_ccr(env) << 32) |
> -        ((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
> -        cpu_get_cwp64(env);
> +    tsptr->tstate = sparc64_tstate(env);
>      tsptr->tpc = env->pc;
>      tsptr->tnpc = env->npc;
>      tsptr->tt = intno;
> @@ -148,7 +146,6 @@ void sparc_cpu_do_interrupt(CPUState *cs)
>      }
>  
>      if (env->def.features & CPU_FEATURE_GL) {
> -        tsptr->tstate |= (env->gl & 7ULL) << 40;
>          cpu_gl_switch_gregs(env, env->gl + 1);
>          env->gl++;
>      }
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent

Re: [PATCH v2 0/4] linux/sparc: more get/set_context fixes

[Peter Maydell]

On Fri, 6 Nov 2020 at 15:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Based-on: 20201105212314.9628-1-peter.maydell@linaro.org
> ("[PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs")
>
> This series fixes a few more issues with our sparc linux-user
> sparc64_get_context() and sparc64_set_context() implementation:
>  * we weren't handling FPU regs correctly, and also the way
>    we coded the handling triggered Coverity warnings
>  * some stray pointless error checks
>  * we shouldn't restore %g7 in set_context
>  * we weren't saving and restoring tstate correctly

The 'v2' in the subject tag is wrong, incidentally; this is
the first version of this series :-)

-- PMM