On 17.07.2015 17:39, Christoffer Dall wrote: > On Fri, Jul 17, 2015 at 03:29:56PM +0100, Peter Maydell wrote: >> On 16 July 2015 at 12:34, Christoffer Dall <christoffer.dall@xxxxxxxxxx> 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 <christoffer.dall@xxxxxxxxxx> >>> --- >>> 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 > Hi, I can if you want check if this patch actually fixes the problem without the KVM workaround. Is this the version I am supposed to test, or should I wait for the next respin? Thanks, Claudio -- Claudio Fontana Server Virtualization Architect Huawei Technologies Duesseldorf GmbH Riesstraße 25 - 80992 München _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
