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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.



-- 
Alexey