[PATCH] target/sparc: Set reg window data structures currently after vmstate load

[PATCH] target/sparc: Set reg window data structures currently after vmstate load

[Peter Maydell]

In the SPARC CPU state, env->regwptr points into the env->regbase
array at wherever the architectural CWP (current window pointer) says
we are in the register windows.  We don't migrate this directly,
since it's a host pointer, so we must ensure it is set up again
after migration load.

We also have to deal with a special case when CWP is (nwindows - 1).
In this case, while running we keep the "in" register data for this
window in a temporary location at the end of the regbase[] array, so
that generated code doesn't have to special case this "wrap around"
case.  In cpu_pre_save() we call cpu_set_cwp() to force a copy of the
wrapped data from its temporary location into the architectural
location in window 0's "out" registers.  We then migrate only
(nwindows * 16) entries in the regbase[] array.  So on the
destination we need to copy the "in" register data back to its
temporary location again.

For 32-bit SPARC we get this right, because the CWP is in the PSR.
The get_psr() function does:
    env->cwp = 0;
    cpu_put_psr_raw(env, val);
which causes cpu_put_psr_raw() to call cpu_set_cwp() in a way that
sets up both regwptr and the wrapped-register data.

However, for 64-bit SPARC the CWP is not in the PSR, and
cpu_put_psr_raw() will not call cpu_set_cwp().  This leaves the guest
register state in a corrupted state, and the guest will likely crash
on the destination if it didn't happen to be executing with CWP == 0.

Fix this by adding a cpu_post_load hook which does the cpu_set_cwp()
for the 64-bit case, and enough commentary to explain what is going
on.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I ran into this because the "sparc64-migration" functional test seems
to hit this reliably when built with the address sanitizer.  This is
probably a timing thing, because it doesn't trip any ASAN failures,
the guest just crashes on the destination side most of the time
because it returns from a subroutine to a corrupt %o7 return address.

As far as I can tell this is what the problem is, but I'm a bit
surprised that we have never hit this, given how likely you are to
hit it on a vmsave/vmload or migration.  So am I missing something?
---
 target/sparc/machine.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/target/sparc/machine.c b/target/sparc/machine.c
index 4dd75aff74..b935c31630 100644
--- a/target/sparc/machine.c
+++ b/target/sparc/machine.c
@@ -167,14 +167,50 @@ static int cpu_pre_save(void *opaque)
     SPARCCPU *cpu = opaque;
     CPUSPARCState *env = &cpu->env;
 
-    /* if env->cwp == env->nwindows - 1, this will set the ins of the last
-     * window as the outs of the first window
+    /*
+     * If env->cwp == env->nwindows - 1, this will set the ins of the last
+     * window as the outs of the first window. We do this because we only
+     * transfer the (nwindows * 16) entries of regbase[] that correspond
+     * to the "proper" locations of the registers; this call has the
+     * effect of copying the wrap register values from their temporary
+     * location down to their proper location ready for migration.
      */
     cpu_set_cwp(env, env->cwp);
 
     return 0;
 }
 
+static int cpu_post_load(void *opaque, int version_id)
+{
+    /*
+     * For 32-bit SPARC we set the register window data structures
+     * up again correctly when get_psr() called cpu_put_psr_raw(),
+     * which calls cpu_set_cwp(). But for 64-bit SPARC the CWP isn't
+     * part of the PSR, so we need to handle it here.
+     *
+     * There are two things we need to have happen:
+     * 1. regwptr points into regbase[] at the location determined by CWP.
+     * 2. If CWP is (nwindows - 1) then we keep the "in" regs of this window
+     *    in a copy at a temporary location at the end of the regbase[]
+     *    array. For migration we only transferred the (nwindows * 16)
+     *    entries of regbase[] that correspond to the "proper" locations
+     *    of the registers, so now we must copy the wrap registers back to
+     *    their temporary location.
+     *
+     * We achieve both of these by setting env->cwp to 0 and then calling
+     * cpu_set_cwp(). This is the same thing get_psr() does for 32-bit.
+     */
+#if defined(TARGET_SPARC64)
+    SPARCCPU *cpu = opaque;
+    CPUSPARCState *env = &cpu->env;
+    uint32_t cwp = env->cwp;
+
+    env->cwp = 0;
+    cpu_set_cwp(env, cwp);
+#endif
+    return 0;
+}
+
 /* 32-bit SPARC retains migration compatibility with older versions
  * of QEMU; 64-bit SPARC has had a migration break since then, so the
  * versions are different.
@@ -190,6 +226,7 @@ const VMStateDescription vmstate_sparc_cpu = {
     .version_id = SPARC_VMSTATE_VER,
     .minimum_version_id = SPARC_VMSTATE_VER,
     .pre_save = cpu_pre_save,
+    .post_load = cpu_post_load,
     .fields = (const VMStateField[]) {
         VMSTATE_UINTTL_ARRAY(env.gregs, SPARCCPU, 8),
         VMSTATE_UINT32(env.nwindows, SPARCCPU),
-- 
2.43.0

Re: [PATCH] target/sparc: Set reg window data structures currently after vmstate load

[Mark Cave-Ayland]

On 13/03/2026 17:09, Peter Maydell wrote:

> In the SPARC CPU state, env->regwptr points into the env->regbase
> array at wherever the architectural CWP (current window pointer) says
> we are in the register windows.  We don't migrate this directly,
> since it's a host pointer, so we must ensure it is set up again
> after migration load.
> 
> We also have to deal with a special case when CWP is (nwindows - 1).
> In this case, while running we keep the "in" register data for this
> window in a temporary location at the end of the regbase[] array, so
> that generated code doesn't have to special case this "wrap around"
> case.  In cpu_pre_save() we call cpu_set_cwp() to force a copy of the
> wrapped data from its temporary location into the architectural
> location in window 0's "out" registers.  We then migrate only
> (nwindows * 16) entries in the regbase[] array.  So on the
> destination we need to copy the "in" register data back to its
> temporary location again.
> 
> For 32-bit SPARC we get this right, because the CWP is in the PSR.
> The get_psr() function does:
>      env->cwp = 0;
>      cpu_put_psr_raw(env, val);
> which causes cpu_put_psr_raw() to call cpu_set_cwp() in a way that
> sets up both regwptr and the wrapped-register data.
> 
> However, for 64-bit SPARC the CWP is not in the PSR, and
> cpu_put_psr_raw() will not call cpu_set_cwp().  This leaves the guest
> register state in a corrupted state, and the guest will likely crash
> on the destination if it didn't happen to be executing with CWP == 0.
> 
> Fix this by adding a cpu_post_load hook which does the cpu_set_cwp()
> for the 64-bit case, and enough commentary to explain what is going
> on.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I ran into this because the "sparc64-migration" functional test seems
> to hit this reliably when built with the address sanitizer.  This is
> probably a timing thing, because it doesn't trip any ASAN failures,
> the guest just crashes on the destination side most of the time
> because it returns from a subroutine to a corrupt %o7 return address.
> 
> As far as I can tell this is what the problem is, but I'm a bit
> surprised that we have never hit this, given how likely you are to
> hit it on a vmsave/vmload or migration.  So am I missing something?
> ---
>   target/sparc/machine.c | 41 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/target/sparc/machine.c b/target/sparc/machine.c
> index 4dd75aff74..b935c31630 100644
> --- a/target/sparc/machine.c
> +++ b/target/sparc/machine.c
> @@ -167,14 +167,50 @@ static int cpu_pre_save(void *opaque)
>       SPARCCPU *cpu = opaque;
>       CPUSPARCState *env = &cpu->env;
>   
> -    /* if env->cwp == env->nwindows - 1, this will set the ins of the last
> -     * window as the outs of the first window
> +    /*
> +     * If env->cwp == env->nwindows - 1, this will set the ins of the last
> +     * window as the outs of the first window. We do this because we only
> +     * transfer the (nwindows * 16) entries of regbase[] that correspond
> +     * to the "proper" locations of the registers; this call has the
> +     * effect of copying the wrap register values from their temporary
> +     * location down to their proper location ready for migration.
>        */
>       cpu_set_cwp(env, env->cwp);
>   
>       return 0;
>   }
>   
> +static int cpu_post_load(void *opaque, int version_id)
> +{
> +    /*
> +     * For 32-bit SPARC we set the register window data structures
> +     * up again correctly when get_psr() called cpu_put_psr_raw(),
> +     * which calls cpu_set_cwp(). But for 64-bit SPARC the CWP isn't
> +     * part of the PSR, so we need to handle it here.
> +     *
> +     * There are two things we need to have happen:
> +     * 1. regwptr points into regbase[] at the location determined by CWP.
> +     * 2. If CWP is (nwindows - 1) then we keep the "in" regs of this window
> +     *    in a copy at a temporary location at the end of the regbase[]
> +     *    array. For migration we only transferred the (nwindows * 16)
> +     *    entries of regbase[] that correspond to the "proper" locations
> +     *    of the registers, so now we must copy the wrap registers back to
> +     *    their temporary location.
> +     *
> +     * We achieve both of these by setting env->cwp to 0 and then calling
> +     * cpu_set_cwp(). This is the same thing get_psr() does for 32-bit.
> +     */
> +#if defined(TARGET_SPARC64)
> +    SPARCCPU *cpu = opaque;
> +    CPUSPARCState *env = &cpu->env;
> +    uint32_t cwp = env->cwp;
> +
> +    env->cwp = 0;
> +    cpu_set_cwp(env, cwp);
> +#endif
> +    return 0;
> +}
> +
>   /* 32-bit SPARC retains migration compatibility with older versions
>    * of QEMU; 64-bit SPARC has had a migration break since then, so the
>    * versions are different.
> @@ -190,6 +226,7 @@ const VMStateDescription vmstate_sparc_cpu = {
>       .version_id = SPARC_VMSTATE_VER,
>       .minimum_version_id = SPARC_VMSTATE_VER,
>       .pre_save = cpu_pre_save,
> +    .post_load = cpu_post_load,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINTTL_ARRAY(env.gregs, SPARCCPU, 8),
>           VMSTATE_UINT32(env.nwindows, SPARCCPU),

Nice analysis! I suspect the reason this doesn't always show up is because any 
context switch (e.g. OpenBIOS, kernel to user, 32-bit to 64-bit etc.) will flush the 
entire register window set to the stack, and hence reset env->cwp back to 0.

It does instinctively feel odd that 32-bit and 64-bit aren't handled in the same way 
- I wonder if this implicit behaviour on 32-bit somehow needs to be more explicit?


ATB,

Mark.

Re: [PATCH] target/sparc: Set reg window data structures currently after vmstate load

[Peter Maydell]

On Mon, 16 Mar 2026 at 21:42, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 13/03/2026 17:09, Peter Maydell wrote:
>
> > In the SPARC CPU state, env->regwptr points into the env->regbase
> > array at wherever the architectural CWP (current window pointer) says
> > we are in the register windows.  We don't migrate this directly,
> > since it's a host pointer, so we must ensure it is set up again
> > after migration load.
> >
> > We also have to deal with a special case when CWP is (nwindows - 1).
> > In this case, while running we keep the "in" register data for this
> > window in a temporary location at the end of the regbase[] array, so
> > that generated code doesn't have to special case this "wrap around"
> > case.  In cpu_pre_save() we call cpu_set_cwp() to force a copy of the
> > wrapped data from its temporary location into the architectural
> > location in window 0's "out" registers.  We then migrate only
> > (nwindows * 16) entries in the regbase[] array.  So on the
> > destination we need to copy the "in" register data back to its
> > temporary location again.
> >
> > For 32-bit SPARC we get this right, because the CWP is in the PSR.
> > The get_psr() function does:
> >      env->cwp = 0;
> >      cpu_put_psr_raw(env, val);
> > which causes cpu_put_psr_raw() to call cpu_set_cwp() in a way that
> > sets up both regwptr and the wrapped-register data.
> >
> > However, for 64-bit SPARC the CWP is not in the PSR, and
> > cpu_put_psr_raw() will not call cpu_set_cwp().  This leaves the guest
> > register state in a corrupted state, and the guest will likely crash
> > on the destination if it didn't happen to be executing with CWP == 0.
> >
> > Fix this by adding a cpu_post_load hook which does the cpu_set_cwp()
> > for the 64-bit case, and enough commentary to explain what is going
> > on.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > I ran into this because the "sparc64-migration" functional test seems
> > to hit this reliably when built with the address sanitizer.  This is
> > probably a timing thing, because it doesn't trip any ASAN failures,
> > the guest just crashes on the destination side most of the time
> > because it returns from a subroutine to a corrupt %o7 return address.
> >
> > As far as I can tell this is what the problem is, but I'm a bit
> > surprised that we have never hit this, given how likely you are to
> > hit it on a vmsave/vmload or migration.  So am I missing something?
> > ---
> >   target/sparc/machine.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/sparc/machine.c b/target/sparc/machine.c
> > index 4dd75aff74..b935c31630 100644
> > --- a/target/sparc/machine.c
> > +++ b/target/sparc/machine.c
> > @@ -167,14 +167,50 @@ static int cpu_pre_save(void *opaque)
> >       SPARCCPU *cpu = opaque;
> >       CPUSPARCState *env = &cpu->env;
> >
> > -    /* if env->cwp == env->nwindows - 1, this will set the ins of the last
> > -     * window as the outs of the first window
> > +    /*
> > +     * If env->cwp == env->nwindows - 1, this will set the ins of the last
> > +     * window as the outs of the first window. We do this because we only
> > +     * transfer the (nwindows * 16) entries of regbase[] that correspond
> > +     * to the "proper" locations of the registers; this call has the
> > +     * effect of copying the wrap register values from their temporary
> > +     * location down to their proper location ready for migration.
> >        */
> >       cpu_set_cwp(env, env->cwp);
> >
> >       return 0;
> >   }
> >
> > +static int cpu_post_load(void *opaque, int version_id)
> > +{
> > +    /*
> > +     * For 32-bit SPARC we set the register window data structures
> > +     * up again correctly when get_psr() called cpu_put_psr_raw(),
> > +     * which calls cpu_set_cwp(). But for 64-bit SPARC the CWP isn't
> > +     * part of the PSR, so we need to handle it here.
> > +     *
> > +     * There are two things we need to have happen:
> > +     * 1. regwptr points into regbase[] at the location determined by CWP.
> > +     * 2. If CWP is (nwindows - 1) then we keep the "in" regs of this window
> > +     *    in a copy at a temporary location at the end of the regbase[]
> > +     *    array. For migration we only transferred the (nwindows * 16)
> > +     *    entries of regbase[] that correspond to the "proper" locations
> > +     *    of the registers, so now we must copy the wrap registers back to
> > +     *    their temporary location.
> > +     *
> > +     * We achieve both of these by setting env->cwp to 0 and then calling
> > +     * cpu_set_cwp(). This is the same thing get_psr() does for 32-bit.
> > +     */
> > +#if defined(TARGET_SPARC64)
> > +    SPARCCPU *cpu = opaque;
> > +    CPUSPARCState *env = &cpu->env;
> > +    uint32_t cwp = env->cwp;
> > +
> > +    env->cwp = 0;
> > +    cpu_set_cwp(env, cwp);
> > +#endif
> > +    return 0;
> > +}
> > +
> >   /* 32-bit SPARC retains migration compatibility with older versions
> >    * of QEMU; 64-bit SPARC has had a migration break since then, so the
> >    * versions are different.
> > @@ -190,6 +226,7 @@ const VMStateDescription vmstate_sparc_cpu = {
> >       .version_id = SPARC_VMSTATE_VER,
> >       .minimum_version_id = SPARC_VMSTATE_VER,
> >       .pre_save = cpu_pre_save,
> > +    .post_load = cpu_post_load,
> >       .fields = (const VMStateField[]) {
> >           VMSTATE_UINTTL_ARRAY(env.gregs, SPARCCPU, 8),
> >           VMSTATE_UINT32(env.nwindows, SPARCCPU),
>
> Nice analysis! I suspect the reason this doesn't always show up is because any
> context switch (e.g. OpenBIOS, kernel to user, 32-bit to 64-bit etc.) will flush the
> entire register window set to the stack, and hence reset env->cwp back to 0.
>
> It does instinctively feel odd that 32-bit and 64-bit aren't handled in the same way
> - I wonder if this implicit behaviour on 32-bit somehow needs to be more explicit?

I did wonder about that, but I preferred leaving the 32-bit handling
the way it is -- trying to unify it with 64-bit would require making
the get_psr() code path go out of its way to avoid updating the
reg window data structures, which seems awkward and a bit unnecessary
given the code does work. So I opted to describe the way the 32-bit
targets do it in the cpu_post_load() comment, so at least we have
a record of it.

Are you OK with this patch as it stands, or would you like me to
change it?

thanks
-- PMM