From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2 00/23] MIPS: KVM: Fixes and guest timer rewrite Date: Thu, 29 May 2014 19:03:12 +0200 Message-ID: <53876850.20600@redhat.com> References: <1401355005-20370-1-git-send-email-james.hogan@imgtec.com> <53870DAD.7050900@redhat.com> <53874719.5070604@imgtec.com> <538750F8.7040202@redhat.com> <53875FEE.1020607@imgtec.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Cc: Andreas Herrmann , Gleb Natapov , kvm@vger.kernel.org, Ralf Baechle , linux-mips@linux-mips.org, David Daney , Sanjay Lal To: James Hogan Return-path: Received: from mx1.redhat.com ([209.132.183.28]:28675 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756178AbaE2REk (ORCPT ); Thu, 29 May 2014 13:04:40 -0400 In-Reply-To: <53875FEE.1020607@imgtec.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 29/05/2014 18:27, James Hogan ha scritto: > (although as it stands CP0_Count never represents the offset from the VM > clock for KVM like it does with a running Count with TCG, so the vmstate > is technically incompatible between TCG/KVM). That can be fixed in cpu_save/cpu_load hooks, like if (kvm_enabled()) { uint32_t TCGlike_CP0_Count = ... qemu_put_sbe32s(f, &TCGlike_CP0_Count); } else { qemu_put_sbe32s(f, &env->CP0_Count); } ... if (kvm_enabled()) { uint32_t TCGlike_CP0_Count; qemu_get_sbe32s(f, &TCGlike_CP0_Count); env->CP0_Count = ... } else { qemu_get_sbe32s(f, &env->CP0_Count); } >> Also, perhaps this bit in kvm_mips_restore_count is unnecessary, and so >> is env->count_save_time in general: >> >>> + /* find time to resume the saved timer at */ >>> + now = get_clock(); >>> + count_resume = now - (cpu_get_clock_at(now) - >>> env->count_save_time); >> >> Is the COUNT_RESUME write necessary if the VM is running? > > Running at that instant or running continuously since the save? > > At this instant the VM is always running. Either it's just been started > and other state isn't dirty, or the registers have been put while the VM > is running. The possible transitions are: running, not dirty -> stopped need to freeze and load the registers stopped -> running, not dirty will reload the registers, need to modify COUNT_RESUME running, dirty -> stopped no need to do anything stopped -> running, dirty will not reload the registers until put, will need to modify COUNT_RESUME on the next transition to "running, not dirty" running, not dirty -> running, dirty need to freeze and load the registers running, dirty -> running, not dirty need to modify COUNT_RESUME if the machine had been stopped in the meanwhile The questions then is, can we skip tracking count_save_time and modifying COUNT_RESUME in kvm_mips_restore_count? Then you can just write get_clock() to COUNT_RESUME in kvm_mips_update_state, like this: if (!running) { if (!cs->kvm_vcpu_dirty) { save; } else { write get_clock() to COUNT_RESUME; if (!cs->kvm_vcpu_dirty) { restore; } } and even drop patch 1. COUNT_RESUME is not even ever read by QEMU nor stored in CPUState, so. The difference is that the guest "loses" the time between the "running, not dirty -> running, dirty" and "running, dirty -> stopped" transitions, while "gaining" the time between "stopped -> running, dirty" and "running, dirty -> running, not dirty". If this is right, I think the difference does not matter in practice and the new/simpler code even explains the definition of COUNT_RESUME better in my eyes. >> Does the >> master disable bit just latch the values, or does it really stop the >> timer? (My reading of the code is the former, since writing >> COUNT_RESUME only modifies the bias: no write => no bias change => timer >> runs). > > It appears latched in the sense that starting it again will jump Count > forward to the time it would have been had it not been disabled (with no > loss of Compare interrupt in that time). Yes, this is the important part because it means that the guest clock does not get progressively more skewed. It also means that it is right to never write COUNT_RESUME except if you go through stop/continue. Paolo