[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 01/10] hw/mips/cputimer: Don't start periodic
From: |
James Hogan |
Subject: |
Re: [Qemu-devel] [PATCH v4 01/10] hw/mips/cputimer: Don't start periodic timer in KVM mode |
Date: |
Thu, 20 Mar 2014 09:57:53 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
On 19/03/14 16:29, Paolo Bonzini wrote:
> Il 14/03/2014 13:47, James Hogan ha scritto:
>> From: Sanjay Lal <address@hidden>
>>
>> Compare/Count timer interrupts are handled in-kernel for KVM, so don't
>> bother starting it in QEMU.
>>
>> Signed-off-by: Sanjay Lal <address@hidden>
>> Signed-off-by: James Hogan <address@hidden>
>> Reviewed-by: Aurelien Jarno <address@hidden>
>> ---
>> 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;
You mean because of the memset in object_initialize_with_type, when
object_new is called? Although that wouldn't handle reset, although
technically the reset state of Compare is undefined.
> 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.
Okay, makes sense.
>
> 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).
Agreed. I'm in the middle of fixing the count/compare timer in KVM to be
based on real time (ktime_get()), so I'll make it default to 100MHz to
match QEMU for now. I can imagine it being useful to be able to control
it too depending on whether you're running on a slow FPGA/emulator or
fast silicon.
> 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.
True, I hadn't considered qemu writing those registers yet.
Am I right that the correct way to prevent clock drift is for
kvm_arch_put_registers to only set the Count register if level !=
KVM_PUT_RUNTIME_STATE?
> 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?
Yes, you appear to be right!
Thanks a lot for reviewing
Cheers
James
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4 00/10] KVM Support for MIPS32 Processors, James Hogan, 2014/03/14
- [Qemu-devel] [PATCH v4 02/10] hw/mips: Add API to convert KVM guest KSEG0 <-> GPA, James Hogan, 2014/03/14
- [Qemu-devel] [PATCH v4 03/10] target-mips: get_physical_address: Add defines for segment bases, James Hogan, 2014/03/14
- [Qemu-devel] [PATCH v4 05/10] kvm: Allow arch to set sigmask length, James Hogan, 2014/03/14
- [Qemu-devel] [PATCH v4 04/10] target-mips: get_physical_address: Add KVM awareness, James Hogan, 2014/03/14
- [Qemu-devel] [PATCH v4 07/10] hw/mips: In KVM mode, inject IRQ2 (I/O) interupts via ioctls, James Hogan, 2014/03/14
- [Qemu-devel] [PATCH v4 08/10] hw/mips: malta: Add KVM support, James Hogan, 2014/03/14