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

[Laurent Vivier <lvivier@redhat.com>]

On 13/09/2017 19:11, Mark Cave-Ayland wrote:
> On 13/09/17 08:12, David Gibson wrote:
> 
>> 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.
> 
> I know that we discussed this in-depth last year, however I was working
> along the lines that Laurent's patch fixed this along the lines of our
> previous discussion:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00338.html (and
> indeed Laurent's analysis at
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg01489.html).
> 
> However looking again at the this patch in the context you mentioned
> above, I'm starting to wonder if the right solution now is for the
> machine init function for g3beige/mac99 to do the same as spapr, e.g.
> add cpu_ppc_clock_vm_state_change() as a vm_change_state_handler and
> then add VMSTATE_PPC_TIMEBASE_V from the machine PPCTimeBase into my new
> subsection?
> 
> Laurent, do you think that your state change handler would work
> correctly in this way?

I think all is explained in the second link you have mentioned, it seems 
we don't need a state handler as KVM DECR will no be updated by the migrated value:

hw/ppc/ppc.c

    736 static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
    737                                  QEMUTimer *timer,
    738                                  void (*raise_excp)(void *),
    739                                  void (*lower_excp)(PowerPCCPU *),
    740                                  uint32_t decr, uint32_t value)
    741 {
...
    749     if (kvm_enabled()) {
    750         /* KVM handles decrementer exceptions, we don't need our own timer */
    751         return;
    752     }
...

But this allows to migrate it for TCG. And it should be correct because in case of TCG I think [need to check] timebase is stopped too (so offset is 0)

David, do you agree with that?

Thanks,
Laurent