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

[Alexey Kardashevskiy]

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.





> Signed-off-by: Mark Cave-Ayland <address@hidden>
> ---
>   target-ppc/machine.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index cb56423..5ee6269 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -499,6 +499,54 @@ static const VMStateDescription vmstate_tlbmas = {
>       }
>   };
>
> +static bool decr_needed(void *opaque)
> +{
> +    return true;
> +}
> +
> +static int get_decr_state(QEMUFile *f, void *opaque, size_t size)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_ppc_store_decr(env, qemu_get_be32(f));
> +
> +    return 0;
> +}
> +
> +static void put_decr_state(QEMUFile *f, void *opaque, size_t size)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    qemu_put_be32(f, cpu_ppc_load_decr(env));
> +}
> +
> +static const VMStateInfo vmstate_info_decr = {
> +    .name = "decr_state",
> +    .get = get_decr_state,
> +    .put = put_decr_state
> +};
> +
> +static const VMStateDescription vmstate_decr = {
> +    .name = "cpu/decr",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = decr_needed,
> +    .fields = (VMStateField[]) {
> +        {
> +            .name         = "cpu/decr",
> +            .version_id   = 0,
> +            .field_exists = NULL,
> +            .size         = 0,
> +            .info         = &vmstate_info_decr,
> +            .flags        = VMS_SINGLE,
> +            .offset       = 0,
> +        },
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   const VMStateDescription vmstate_ppc_cpu = {
>       .name = "cpu",
>       .version_id = 5,
> @@ -555,6 +603,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>           &vmstate_tlb6xx,
>           &vmstate_tlbemb,
>           &vmstate_tlbmas,
> +        &vmstate_decr,
>           NULL
>       }
>   };


--
Alexey