Re: [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration

[David Gibson]

On Mon, Jan 11, 2016 at 07:43:54AM +0000, Mark Cave-Ayland wrote:
> On 11/01/16 04:55, David Gibson wrote:
> 
> > On Mon, Jan 11, 2016 at 12:18:31PM +1100, Alexey Kardashevskiy wrote:
> >> On 01/09/2016 01:21 AM, Mark Cave-Ayland wrote:
> >>> On 08/01/16 02:47, Alexey Kardashevskiy wrote:
> >>>
> >>>> On 01/07/2016 05:22 AM, Mark Cave-Ayland wrote:
> >>>>> During local testing with TCG, intermittent errors were found when
> >>>>> trying to
> >>>>> migrate Darwin OS images.
> >>>>>
> >>>>> The underlying cause was that Darwin resets the decrementer value to
> >>>>> fairly
> >>>>> small values on each interrupt. cpu_ppc_set_tb_clk() sets the default
> >>>>> value
> >>>>> of the decrementer to 0xffffffff during initialisation which typically
> >>>>> corresponds to several seconds. Hence when restoring the image, the 
> >>>>> guest
> >>>>> would effectively "lose" decrementer interrupts during this time causing
> >>>>> confusion in the guest.
> >>>>>
> >>>>> NOTE: there does seem to be some overlap here with the
> >>>>> vmstate_ppc_timebase
> >>>>> code, however it doesn't seem to handle multiple CPUs which is why
> >>>>> I've gone
> >>>>> for an independent implementation.
> >>>>
> >>>> It does handle multiple CPUs:
> >>>>
> >>>> static int timebase_post_load(void *opaque, int version_id)
> >>>> {
> >>>> ...
> >>>>      /* Set new offset to all CPUs */
> >>>>      CPU_FOREACH(cpu) {
> >>>>          PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> >>>>          pcpu->env.tb_env->tb_offset = tb_off_adj;
> >>>>      }
> >>>>
> >>>>
> >>>> It does not transfer DECR though, it transfers the offset instead, one
> >>>> for all CPUs.
> >>>>
> >>>>
> >>>> The easier solution would be just adding this instead of the whole patch:
> >>>>
> >>>> spr_register(env, SPR_DECR, "DECR",
> >>>>              SPR_NOACCESS, SPR_NOACCESS,
> >>>>              &spr_read_decr, &spr_write_decr,
> >>>>              0x00000000);
> >>>>
> >>>> somewhere in target-ppc/translate_init.c for CPUs you want to support,
> >>>> gen_tbl() seems to be the right place for this. As long as it is just
> >>>> spr_register() and not spr_register_kvm(), I suppose it should not break
> >>>> KVM and pseries.
> >>>
> >>> I've just tried adding that but it then gives the following error on
> >>> startup:
> >>>
> >>> Error: Trying to register SPR 22 (016) twice !
> >>>
> >>> Based upon this, the existing registration seems fine. I think the
> >>> problem is that I can't see anything in __cpu_ppc_store_decr() that
> >>> updates the spr[SPR_DECR] value when the decrementer register is
> >>> changed, so it needs to be explicitly added to
> >>> cpu_pre_save()/cpu_post_load():
> >>>
> >>>
> >>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> >>> index 251a84b..495e58d 100644
> >>> --- a/target-ppc/machine.c
> >>> +++ b/target-ppc/machine.c
> >>> @@ -141,6 +141,7 @@ static void cpu_pre_save(void *opaque)
> >>>      env->spr[SPR_CFAR] = env->cfar;
> >>>  #endif
> >>>      env->spr[SPR_BOOKE_SPEFSCR] = env->spe_fscr;
> >>> +    env->spr[SPR_DECR] = cpu_ppc_load_decr(env);
> >>>
> >>>      for (i = 0; (i < 4) && (i < env->nb_BATs); i++) {
> >>>          env->spr[SPR_DBAT0U + 2*i] = env->DBAT[0][i];
> >>> @@ -175,6 +176,7 @@ static int cpu_post_load(void *opaque, int version_id)
> >>>      env->cfar = env->spr[SPR_CFAR];
> >>>  #endif
> >>>      env->spe_fscr = env->spr[SPR_BOOKE_SPEFSCR];
> >>> +    cpu_ppc_store_decr(env, env->spr[SPR_DECR]);
> >>>
> >>>      for (i = 0; (i < 4) && (i < env->nb_BATs); i++) {
> >>>          env->DBAT[0][i] = env->spr[SPR_DBAT0U + 2*i];
> >>>
> >>>
> >>> I've just tried the diff above instead of my original version and
> >>> repeated my savevm/loadvm pair test with a Darwin installation and it
> >>> also fixes the random hang I was seeing on loadvm.
> >>>
> >>> Seemingly this should work on KVM in that cpu_ppc_load_decr() and
> >>> cpu_ppc_store_decr() become no-ops as long as KVM maintains
> >>> env->spr[SPR_DECR], but a second set of eyeballs would be useful here.
> >>>
> >>> If you can let me know if this is suitable then I'll update the patchset
> >>> based upon your feedback and send out a v2.
> >>
> >>
> >> Looks good to me, I'd just wait a day or two in the case if David wants to
> >> comment.
> > 
> > I was on holiday and missed the start of this thread, I'm afraid, so I
> > don't fully understand the context here.
> 
> It's part of a patchset I posted which fixes up problems I had with
> migrating g3beige/mac99 machines under TCG:
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00544.html.
> 
> Apologies for not adding you as CC directly - are you still helping to
> cover ppc-next for Alex?

Yes, I am.

> > Am I right in thinking that this change will essentially freeze the
> > decrementer across the migration downtime?  That doesn't seem right,
> > since the decrementer is supposed to be linked to the timebase and
> > represent real time passing.
> 
> Yes, that's correct.
> 
> > In other words, isn't this just skipping the decrementer interrupts at
> > the qemu level rather than the guest level?
> > 
> > It seems that instead we should be reconstructing the decrementer on
> > the destination based on an offset from the timebase.
> 
> Well I haven't really looked at how time warping works during in
> migration for QEMU, however this seems to be the method used by
> hw/ppc/ppc.c's timebase_post_load() function but my understanding is
> that this isn't currently available for the g3beige/mac99 machines?

Ah.. yes, it looks like the timebase migration stuff is only hooked in
on the pseries machine type.  As far as I can tell it should be
trivial to add it to other machines though - it doesn't appear to rely
on anything outside the common ppc timebase stuff.

> Should the patch in fact do this but also add decrementer support? And
> if it did, would this have a negative effect on pseries?

Yes, I think that's the right approach.  Note that rather than
duplicating the logic to adjust the decrementer over migration, it
should be possible to encode the decrementer as a diff from the
timebase across the migration.

In fact.. I'm not sure it ever makes sense to store the decrementer
value as a direct value, since it's constantly changing - probably
makes more sense to derive it from the timebase whenever it is needed.

As far as I know that should be fine for pseries.  I think the current
behaviour is probably technically wrong for pseries as well, but the
timing code of our Linux guests is robust enough to handle a small
displacement to the time of the next decrementer interrupt.

> But yes, assuming that the guest time warp is handled correctly (which I
> assume is handled correctly elsewhere since this would also be required
> for KVM) then I think that this should work.
> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature