On 18/07/2026 09:46, Peter Maydell wrote:
Ping^3. It would be nice to get this fix into 11.1, since it's
a bug that trips the address-sanitizer.
thanks
-- PMM
On Mon, 11 May 2026 at 15:45, Peter Maydell <[email protected]> wrote:
Ping^2. I think this is now the only remaining blocker to being
able to do a full sanitizer test run. I'm happy to throw it into
my target-arm queue if you don't have any other sparc patches you
need to send, but I think it would be better if it had a
reviewed-by tag first :-)
-- PMM
On Thu, 30 Apr 2026 at 14:02, Peter Maydell <[email protected]> wrote:
Ping ? Now we're in the 11.1 cycle it would be nice to get these
remaining sanitizer errors fixed so we can maybe get a CI job
to test a sanitizer build.
thanks
-- PMM
On Fri, 20 Mar 2026 at 11:02, Peter Maydell <[email protected]> wrote:
On Mon, 16 Mar 2026 at 21:42, Mark Cave-Ayland
<[email protected]> 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: [email protected]
Signed-off-by: Peter Maydell <[email protected]>
---
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?
Sorry about the long delay getting to this one. Looking through the migration code,
the current approach uses get/put handlers for registers that need to do more than
just update value in the state. Longer term that code could do with an update to use
.post_load, but for now I think it makes sense to use the existing pattern,
particularly as it keeps the separation between 32-bit and 64-bit.
The following diff gives the same output in analyze-migration.py and I think should
solve the issue based upon your analysis:
diff --git a/target/sparc/machine.c b/target/sparc/machine.c
index 5f402e098cf..8fdf1782113 100644
--- a/target/sparc/machine.c
+++ b/target/sparc/machine.c
@@ -151,6 +151,37 @@ static const VMStateInfo vmstate_xcc = {
.get = get_xcc,
.put = put_xcc,
};
+
+static int get_cwp(QEMUFile *f, void *opaque, size_t size,
+ const VMStateField *field)
+{
+ SPARCCPU *cpu = opaque;
+ CPUSPARCState *env = &cpu->env;
+ uint32_t val = qemu_get_be32(f);
+
+ /* needed to ensure that the wrapping registers are correctly updated */
+ env->cwp = 0;
+ cpu_set_cwp(env, val);
+
+ return 0;
+}
+
+static int put_cwp(QEMUFile *f, void *opaque, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc)
+{
+ SPARCCPU *cpu = opaque;
+ CPUSPARCState *env = &cpu->env;
+ uint32_t val = cpu_get_cwp64(env);
+
+ qemu_put_be32(f, val);
+ return 0;
+}
+
+static const VMStateInfo vmstate_cwp = {
+ .name = "uint32",
+ .get = get_cwp,
+ .put = put_cwp,
+};
#else
static bool fq_needed(void *opaque)
{
@@ -286,7 +317,14 @@ const VMStateDescription vmstate_sparc_cpu = {
VMSTATE_CPU_TIMER(env.hstick, SPARCCPU),
/* On SPARC32 env.psrpil and env.cwp are migrated as part of the PSR */
VMSTATE_UINT32(env.psrpil, SPARCCPU),
- VMSTATE_UINT32(env.cwp, SPARCCPU),
+ {
+ .name = "env.cwp",
+ .version_id = 0,
+ .size = sizeof(uint32_t),
+ .info = &vmstate_cwp,
+ .flags = VMS_SINGLE,
+ .offset = 0,
+ },
#endif
VMSTATE_END_OF_LIST()
},
ATB,
Mark.
