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?
