From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG7ir-0003V1-Tz for qemu-devel@nongnu.org; Fri, 17 Jul 2015 11:38:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZG7in-0000zc-My for qemu-devel@nongnu.org; Fri, 17 Jul 2015 11:38:49 -0400 Received: from mail-lb0-f174.google.com ([209.85.217.174]:33973) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG7in-0000yT-BS for qemu-devel@nongnu.org; Fri, 17 Jul 2015 11:38:45 -0400 Received: by lbbzr7 with SMTP id zr7so63412156lbb.1 for ; Fri, 17 Jul 2015 08:38:43 -0700 (PDT) Date: Fri, 17 Jul 2015 17:39:01 +0200 From: Christoffer Dall Message-ID: <20150717153901.GD14024@cbox> References: <1437046488-10773-1-git-send-email-christoffer.dall@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2] target-arm: kvm: Differentiate registers based on write-back levels List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Marc Zyngier , Jan Kiszka , Claudio Fontana , QEMU Developers , "kvmarm@lists.cs.columbia.edu" On Fri, Jul 17, 2015 at 03:29:56PM +0100, Peter Maydell wrote: > On 16 July 2015 at 12:34, Christoffer Dall wrote: > > Some registers like the CNTVCT register should only be written to the > > kernel as part of machine initialization or on vmload operations, but > > never during runtime, as this can potentially make time go backwards or > > create inconsistent time observations between VCPUs. > > > > Introduce a list of registers that should not be written back at runtime > > and check this list on syncing the register state to the KVM state. > > Thanks. I think this should go into QEMU 2.4, given that it fixes > a bug with time misbehaving in guests. Are you happy that it's > received enough testing? (I have given 32-bit KVM a spin but have > no convenient 64-bit box to test with, and besides, I didn't notice the > bug in the first place :-)) I tested this on both Juno and Mustang, with a simple loop kernel booting and doing hackbench test, but if we want to be on the extra careful side, perhaps Alex can run it through his migration test setup? I don't think that's necessary though. > > > Signed-off-by: Christoffer Dall > > --- > > Changes since RFC: > > - Move cpreg_level to kvm_arm_cpreg_level and into kvm32.c and kvm64.c > > - Changed struct name and declare as static const > > I have a couple of minor comments on the comments below, and you > forgot to update the stub version of write_list_to_kvmstate(). > I can just fix these up as I put it into target-arm.next, though. > Fixed up version at: > > https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next > > If interested parties could test that by end-of-Monday that > would be nice (since rc2 is scheduled for Tuesday). > > > dtc | 2 +- > > target-arm/kvm.c | 6 +++++- > > target-arm/kvm32.c | 30 +++++++++++++++++++++++++++++- > > target-arm/kvm64.c | 30 +++++++++++++++++++++++++++++- > > target-arm/kvm_arm.h | 12 +++++++++++- > > target-arm/machine.c | 2 +- > > 6 files changed, 76 insertions(+), 6 deletions(-) > > > > diff --git a/dtc b/dtc > > index 65cc4d2..bc895d6 160000 > > --- a/dtc > > +++ b/dtc > > @@ -1 +1 @@ > > -Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf > > +Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde > > Stray submodule change :-) > Damn, keeps happening to me. ok, I'll stop using git commit -a for qemu changes. > > diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c > > index d7e7d68..6769815 100644 > > --- a/target-arm/kvm32.c > > +++ b/target-arm/kvm32.c > > @@ -153,6 +153,34 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx) > > } > > } > > > > +typedef struct CPRegStateLevel { > > + uint64_t regidx; > > + int level; > > +} CPRegStateLevel; > > + > > +/* All coprocessor registers not listed in the following table are assumed to > > + * be of the level KVM_PUT_RUNTIME_STATE, a register should be written less > > ". If a register". > > > + * often, you must add it to this table with a state of either > > + * KVM_PUT_RESET_STATE or KVM_PUT_FULL_STATE. > > + */ > > +static const CPRegStateLevel non_runtime_cpregs[] = { > > + { KVM_REG_ARM_TIMER_CNT, KVM_PUT_FULL_STATE }, > > +}; > > > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > > index ac34f51..d59f41c 100644 > > --- a/target-arm/kvm64.c > > +++ b/target-arm/kvm64.c > > @@ -139,6 +139,34 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx) > > } > > } > > > > +typedef struct CPRegStateLevel { > > + uint64_t regidx; > > + int level; > > +} CPRegStateLevel; > > + > > +/* All system not listed in the following table are assumed to be of the level > > "system registers" > > > + * KVM_PUT_RUNTIME_STATE, a register should be written less often, you must > > ". If a register" > > > + * add it to this table with a state of either KVM_PUT_RESET_STATE or > > + * KVM_PUT_FULL_STATE. > > + */ > > +static const CPRegStateLevel non_runtime_cpregs[] = { > > + { KVM_REG_ARM_TIMER_CNT, KVM_PUT_FULL_STATE }, > > +}; > > > --- a/target-arm/kvm_arm.h > > +++ b/target-arm/kvm_arm.h > > > @@ -83,7 +93,7 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx); > > * Note that we do not stop early on failure -- we will attempt > > * writing all registers in the list. > > */ > > -bool write_list_to_kvmstate(ARMCPU *cpu); > > +bool write_list_to_kvmstate(ARMCPU *cpu, int level); > > You forgot to update the stub function in target-arm/kvm-stub.c, > so this breaks compilation on non-ARM hosts. > whoops, who cares about non-ARM hosts anyway. Thanks for fixing these up, the fixed up version looks good. -Christoffer