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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On Sun, Sep 10, 2017 at 03:37:35PM +0100, 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.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

This is subtly incorrect.  It sets the DECR on load to exactly the
value that was saved.  That effectively means that the DECR is frozen
for the migration downtime, which probably isn't what we want.

Instead we need to save the DECR as an offset from the timebase, and
restore it relative to the (downtime adjusted) timebase on the
destination.

The complication with that is that the timebase is generally migrated
at the machine level, not the cpu level: the timebase is generally
synchronized between cpus in the machine, and migrating it at the
individual cpu could break that.  Which means we probably need a
machine level hook to handle the decrementer too, even though it
logically *is* per-cpu, because otherwise we'll be trying to restore
it before the timebase is restored.

> ---
>  target/ppc/machine.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 10b3c41..a16a856 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -176,6 +176,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];
> @@ -280,6 +281,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];

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]