[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include th
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration |
Date: |
Tue, 12 Jan 2016 13:44:21 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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
- [Qemu-ppc] [PATCH 3/4] target-ppc: add CPU access_type into the migration stream, (continued)
- [Qemu-ppc] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, Mark Cave-Ayland, 2016/01/06
- Re: [Qemu-ppc] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, Alexey Kardashevskiy, 2016/01/07
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, Mark Cave-Ayland, 2016/01/08
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, Alexey Kardashevskiy, 2016/01/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, David Gibson, 2016/01/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, Mark Cave-Ayland, 2016/01/11
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration,
David Gibson <=
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, Mark Cave-Ayland, 2016/01/15
- Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration, David Gibson, 2016/01/17
- [Qemu-ppc] Migrating decrementer (was: Re: [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration), Mark Cave-Ayland, 2016/01/25
- Re: [Qemu-ppc] Migrating decrementer (was: Re: [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration), David Gibson, 2016/01/25
- Re: [Qemu-ppc] Migrating decrementer (was: Re: [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration), BALATON Zoltan, 2016/01/25
- Re: [Qemu-ppc] Migrating decrementer (was: Re: [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration), David Gibson, 2016/01/26
- Re: [Qemu-ppc] [Qemu-devel] Migrating decrementer, Mark Cave-Ayland, 2016/01/26
- Re: [Qemu-ppc] [Qemu-devel] Migrating decrementer, Mark Cave-Ayland, 2016/01/26
- Re: [Qemu-ppc] [Qemu-devel] Migrating decrementer, David Gibson, 2016/01/31