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
