Re: [Qemu-devel] [RFC 3/4] target-ppc: synchronise tb_offset with KVM host on machine start

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

On Thu, Apr 07, 2016 at 04:23:13PM +0100, Mark Cave-Ayland wrote:
> Recalculate the tb_offset between the guest and host, applying it to all CPUs
> when (re)starting the virtual machine. This has the effect of providing a
> near-seamless virtual timebase for KVM guests that support
> KVM_REG_PPC_TB_OFFSET.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

The upshot of this is to make the timebase effectively stop when the
guest is stopped, yes?  It's not entirely clear that that's the
correct choice.

The timebase is supposed to represent wall clock time.  For debugging
of a virtual machine it's sometimes useful to stop it, but I don't
know that that should be the default behaviour.

I'm also not clear if this includes stops due to migration or
savevm/restore or anything else not explicitly requested as a stop by
the user / management layer.  Those cases absolutely should not stop
the timebase, since that will cause the guest's system clock to get
out of sync with wall clock time.

I'm wondering if we're going to need another option that controls if
the timebase is tied to QEMU_CLOCK_REALTIME or QEMU_CLOCK_VIRTUAL
(analagous to clock=host or clock=vm for the RTC).

> ---
>  hw/ppc/ppc.c |   41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index ccdca5d..39e15b1 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1345,12 +1345,51 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>  }
>  
>  /* Generic PPC machine */
> +static void _ppc_update_timebase(PPCMachineState *pms)
> +{
> +    /* Update guest timebase offset with respect to host */
> +    int64_t tb_off, new_tb_off;
> +    int i;
> +    
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);    
> +    tb_off = first_ppc_cpu->env.tb_env->tb_offset;
> +    
> +    new_tb_off = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                 first_ppc_cpu->env.tb_env->tb_freq, NANOSECONDS_PER_SECOND) +
> +                 tb_off - cpu_get_host_ticks();
> +    
> +    //fprintf(stderr, "tb_off: %" PRIx64 "   new_tb_off: %" PRIx64 "\n", tb_off, new_tb_off);
> +    
> +    /* Set new offset to all CPUs */
> +    for (i = 0; i < smp_cpus; i++) {
> +        PowerPCCPU *pcpu = POWERPC_CPU(qemu_get_cpu(i));
> +        pcpu->env.tb_env->tb_offset = new_tb_off;
> +    }
> +}
> +
> +static void ppc_machine_change_state(void *opaque, int running, RunState state)
> +{
> +    PPCMachineState *s = opaque;
> +
> +    if (running && kvm_enabled()) {      
> +        _ppc_update_timebase(s);
> +    }
> +}
> +
> +static void ppc_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    PPCMachineState *s = g_malloc0(sizeof(PPCMachineState));
> +    
> +    qemu_add_vm_change_state_handler(ppc_machine_change_state, s);
> +}
> +
>  static const TypeInfo ppc_machine_info = {
>      .name = TYPE_PPC_MACHINE,
>      .parent = TYPE_MACHINE,
>      .abstract = true,
>      .instance_size = sizeof(PPCMachineState),
> -    .class_size = sizeof(PPCMachineClass)
> +    .class_size = sizeof(PPCMachineClass),
> +    .class_init = ppc_machine_class_init
>  };
>  
>  static void ppc_machine_register_types(void)

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