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: 3241 bytes --]

On Wed, Sep 13, 2017 at 07:58:23PM +0200, Laurent Vivier wrote:
> 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?

Yes, I think so.  Some details might be different, but the basic idea
of migrating the timebase and decrementers at machine level should be
the same for pseries and g3beige/whatever.

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