Re: [PATCH v4 01/10] hw/mips/cputimer: Don't start periodic timer in KVM mode

[Paolo Bonzini <pbonzini@redhat.com>]

Il 14/03/2014 13:47, James Hogan ha scritto:
> From: Sanjay Lal <sanjayl@kymasys.com>
>
> Compare/Count timer interrupts are handled in-kernel for KVM, so don't
> bother starting it in QEMU.
>
> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> Changes in v2:
>  - Expand commit message
>  - Rebase on v1.7.0
>  - Wrap comment
> ---
>  hw/mips/cputimer.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c
> index c8b4b00..52570fd 100644
> --- a/hw/mips/cputimer.c
> +++ b/hw/mips/cputimer.c
> @@ -23,6 +23,7 @@
>  #include "hw/hw.h"
>  #include "hw/mips/cpudevs.h"
>  #include "qemu/timer.h"
> +#include "sysemu/kvm.h"
>
>  #define TIMER_FREQ	100 * 1000 * 1000
>
> @@ -141,7 +142,13 @@ static void mips_timer_cb (void *opaque)
>
>  void cpu_mips_clock_init (CPUMIPSState *env)
>  {
> -    env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &mips_timer_cb, env);
> -    env->CP0_Compare = 0;
> -    cpu_mips_store_count(env, 1);
> +    /*
> +     * If we're in KVM mode, don't start the periodic timer, that is handled in
> +     * kernel.
> +     */
> +    if (!kvm_enabled()) {
> +        env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &mips_timer_cb, env);
> +        env->CP0_Compare = 0;
> +        cpu_mips_store_count(env, 1);
> +    }
>  }
>

I hate to make you do unrelated changes, but... initializing CP0_Compare 
is unnecessary, it should already be 0; and for CP0_Count it should not 
be done here. but in cpu_state_reset function.  Then here you can call 
qemu_register_reset to register another reset callback, and call 
cpu_mips_timer_update in that callback.

I'm asking because while

     if (!kvm_enabled()) {
         env->timer = ...
         qemu_register_reset(...);
     }

is fine, changing values of registers conditionally is not.

Also, I noticed two things in the implementation of the CPU timer that 
should be fixed:

1) right now the hypervisor's frequency is hardcoded to 1/4th of the 
host, while QEMU's is 100 MHz.  It would be nice to make them either 
consistent, or customizable (you can use another ONE_REG interface to 
set CPU parameters).

2) in KVM, CP0_Count does not start at the same value on guest reset. 
There is a comment that "Linux doesn't seem to write into COUNT", but 
QEMU does.  So KVM should implement CP0_Count writes and adjust the 
"bias" of the guest CP0_Count.

In fact, right now kvm_mips_te_put_cp0_registers should always return 
-EINVAL because KVM_REG_MIPS_CP0_COUNT is not handled in 
kvm_mips_get/set_reg.  Am I missing something?

Paolo