| Message ID | 1442259039-22137-3-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote: > Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription" > appears to drop the internal CPU IRQ state from the migration stream. Whilst > testing migration on g3beige/mac99 machines, test images would randomly fail to > resume unless a key was pressed on the VGA console. > > Further investigation suggests that internal CPU IRQ state isn't being > preserved and so interrupts asserted at the time of migration are lost. Adding > the pending_interrupts and irq_input_state fields back into the migration > stream appears to fix the problem here during local tests. On spapr, interrupt state migrates with XICS interrupt controller and it resets the CPU bits you are adding to the migration descriptor. I'd expect openpic (this one is used for mac99?) to do the same. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > target-ppc/machine.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target-ppc/machine.c b/target-ppc/machine.c > index bd99844..968a7d6 100644 > --- a/target-ppc/machine.c > +++ b/target-ppc/machine.c > @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = { > > /* Internal state */ > VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU), > + VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU), > + VMSTATE_UINT32(env.irq_input_state, PowerPCCPU), This update requires a "version" increment for vmstate_ppc_cpu and VMSTATE_UINT32_V instead of VMSTATE_UINT32. > /* FIXME: access_type? */ > > /* Sanity checking */ >
On 15/09/15 00:10, Alexey Kardashevskiy wrote: > On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote: >> Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription" >> appears to drop the internal CPU IRQ state from the migration stream. >> Whilst >> testing migration on g3beige/mac99 machines, test images would >> randomly fail to >> resume unless a key was pressed on the VGA console. >> >> Further investigation suggests that internal CPU IRQ state isn't being >> preserved and so interrupts asserted at the time of migration are >> lost. Adding >> the pending_interrupts and irq_input_state fields back into the migration >> stream appears to fix the problem here during local tests. > > On spapr, interrupt state migrates with XICS interrupt controller and it > resets the CPU bits you are adding to the migration descriptor. I'd > expect openpic (this one is used for mac99?) to do the same. Interesting. I wrote the patch that converted openpic to VMStateDescription at the end of last year, and my understanding from the feedback was that ideally interrupt state should be maintained so that no post_load function was required. I guess spapr is very different from the basic Mac machines though. Also I see that you also removed the reference to cpu_write_xer() which appears to set some related internal state variables. Is this now not necessary either? >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> target-ppc/machine.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/target-ppc/machine.c b/target-ppc/machine.c >> index bd99844..968a7d6 100644 >> --- a/target-ppc/machine.c >> +++ b/target-ppc/machine.c >> @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = { >> >> /* Internal state */ >> VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU), >> + VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU), >> + VMSTATE_UINT32(env.irq_input_state, PowerPCCPU), > > This update requires a "version" increment for vmstate_ppc_cpu and > VMSTATE_UINT32_V instead of VMSTATE_UINT32. So this means you're happy with the basic patch if I go ahead and make the version changes too? ATB, Mark.
On 09/16/2015 07:09 AM, Mark Cave-Ayland wrote: > On 15/09/15 00:10, Alexey Kardashevskiy wrote: > >> On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote: >>> Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription" >>> appears to drop the internal CPU IRQ state from the migration stream. >>> Whilst >>> testing migration on g3beige/mac99 machines, test images would >>> randomly fail to >>> resume unless a key was pressed on the VGA console. >>> >>> Further investigation suggests that internal CPU IRQ state isn't being >>> preserved and so interrupts asserted at the time of migration are >>> lost. Adding >>> the pending_interrupts and irq_input_state fields back into the migration >>> stream appears to fix the problem here during local tests. >> >> On spapr, interrupt state migrates with XICS interrupt controller and it >> resets the CPU bits you are adding to the migration descriptor. I'd >> expect openpic (this one is used for mac99?) to do the same. > > Interesting. I wrote the patch that converted openpic to > VMStateDescription at the end of last year, and my understanding from > the feedback was that ideally interrupt state should be maintained so > that no post_load function was required. I guess spapr is very different > from the basic Mac machines though. > > Also I see that you also removed the reference to cpu_write_xer() which > appears to set some related internal state variables. Is this now not > necessary either? Not sure here, looks like a bug actually, cpu_post_load() should call it. But it should only affect TCG migration (which we have not extensively tested :) ). >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> target-ppc/machine.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c >>> index bd99844..968a7d6 100644 >>> --- a/target-ppc/machine.c >>> +++ b/target-ppc/machine.c >>> @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = { >>> >>> /* Internal state */ >>> VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU), >>> + VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU), >>> + VMSTATE_UINT32(env.irq_input_state, PowerPCCPU), >> >> This update requires a "version" increment for vmstate_ppc_cpu and >> VMSTATE_UINT32_V instead of VMSTATE_UINT32. > > So this means you're happy with the basic patch if I go ahead and make > the version changes too? Yes, I suppose.
diff --git a/target-ppc/machine.c b/target-ppc/machine.c index bd99844..968a7d6 100644 --- a/target-ppc/machine.c +++ b/target-ppc/machine.c @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = { /* Internal state */ VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU), + VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU), + VMSTATE_UINT32(env.irq_input_state, PowerPCCPU), /* FIXME: access_type? */ /* Sanity checking */
Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription" appears to drop the internal CPU IRQ state from the migration stream. Whilst testing migration on g3beige/mac99 machines, test images would randomly fail to resume unless a key was pressed on the VGA console. Further investigation suggests that internal CPU IRQ state isn't being preserved and so interrupts asserted at the time of migration are lost. Adding the pending_interrupts and irq_input_state fields back into the migration stream appears to fix the problem here during local tests. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- target-ppc/machine.c | 2 ++ 1 file changed, 2 insertions(+)