[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] target-arm: kvm: Differentiate registers bas
From: |
Christoffer Dall |
Subject: |
Re: [Qemu-devel] [PATCH v2] target-arm: kvm: Differentiate registers based on write-back levels |
Date: |
Fri, 17 Jul 2015 17:39:01 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Jul 17, 2015 at 03:29:56PM +0100, Peter Maydell wrote:
> On 16 July 2015 at 12:34, Christoffer Dall <address@hidden> 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 <address@hidden>
> > ---
> > 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