Re: [Qemu-devel] [PATCH v2] target-arm: kvm: Differentiate registers based on write-back levels

[Peter Maydell]

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 :-))

> 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 :-)

> 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.

thanks
-- PMM