Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:
>
> When a VM is stopped (guest is paused) guest virtual time
> should stop counting. Otherwise, when the VM is resumed it
> will experience time jumps and its kernel may report soft
> lockups. Not counting virtual time while the VM is stopped
> has the side effect of making the guest's time appear to lag
> when compared with real time, and even with time derived from
> the physical counter. For this reason, this change, which is
> enabled by default, comes with a KVM CPU feature allowing it
> to be disabled, restoring legacy behavior.
>
> This patch only provides the implementation of the virtual
> time adjustment. A subsequent patch will provide the CPU
> property allowing the change to be enabled and disabled.
>
> Reported-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.h     |  9 +++++++++
>  target/arm/kvm.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/kvm32.c   |  3 +++
>  target/arm/kvm64.c   |  3 +++
>  target/arm/kvm_arm.h | 23 +++++++++++++++++++++
>  5 files changed, 86 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 83a809d4bac4..a79ea74125b3 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -821,6 +821,15 @@ struct ARMCPU {
>      /* KVM init features for this CPU */
>      uint32_t kvm_init_features[7];
>
> +    /* KVM CPU features */
> +    bool kvm_adjvtime;
> +
> +    /* VCPU virtual counter value used with kvm_adjvtime */
> +    uint64_t kvm_vtime;

How does this new state interact with migration ?

> +
> +    /* True if the run state is, or transitioning from, RUN_STATE_PAUSED */
> +    bool runstate_paused;
> +
>      /* Uniprocessor system with MP extensions */
>      bool mp_is_up;
>
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 5b82cefef608..a55fe7d7aefd 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -348,6 +348,24 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
>      memory_region_ref(kd->mr);
>  }
>
> +void kvm_arm_vm_state_change(void *opaque, int running, RunState state)
> +{
> +    CPUState *cs = opaque;
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (running) {
> +        if (cpu->kvm_adjvtime && cpu->runstate_paused) {
> +            kvm_arm_set_virtual_time(cs, cpu->kvm_vtime);
> +        }
> +        cpu->runstate_paused = false;
> +    } else if (state == RUN_STATE_PAUSED) {
> +        cpu->runstate_paused = true;
> +        if (cpu->kvm_adjvtime) {
> +            kvm_arm_get_virtual_time(cs, &cpu->kvm_vtime);
> +        }
> +    }
> +}

How does this interact with the usual register sync to/from
KVM (ie kvm_arch_get_registers(), which I think will do a
GET_ONE_REG read of the TIMER_CNT register the way it does
any other sysreg, inside write_kvmstate_to_list(), plus
kvm_arch_set_registers() which does the write back to the
kernel in write_list_to_kvmstate()) ? Presumably we want this
version to take precedence by the set_virtual_time call
happening after the kvm_arch_set_registers, but is this
guaranteed ?

thanks
-- PMM