Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Peter Maydell]

On Thu, 12 Dec 2019 at 17:33, Andrew Jones <address@hidden> wrote:
>
> kvm-no-adjvtime is a KVM specific CPU property and a first of its kind.
> To accommodate it we also add kvm_arm_add_vcpu_properties() and a
> KVM specific CPU properties description to the CPU features document.
>
> Signed-off-by: Andrew Jones <address@hidden>
> ---
>  docs/arm-cpu-features.rst | 31 ++++++++++++++++++++++++++++++-
>  hw/arm/virt.c             |  8 ++++++++
>  include/hw/arm/virt.h     |  1 +
>  target/arm/cpu.c          |  2 ++
>  target/arm/cpu64.c        |  1 +
>  target/arm/kvm.c          | 28 ++++++++++++++++++++++++++++
>  target/arm/kvm_arm.h      | 11 +++++++++++
>  target/arm/monitor.c      |  1 +
>  tests/arm-cpu-features.c  |  4 ++++
>  9 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
> index 1b367e22e16e..641ec9cb8f4a 100644
> --- a/docs/arm-cpu-features.rst
> +++ b/docs/arm-cpu-features.rst
> @@ -31,7 +31,9 @@ supporting the feature or only supporting the feature under 
> certain
>  configurations.  For example, the `aarch64` CPU feature, which, when
>  disabled, enables the optional AArch32 CPU feature, is only supported
>  when using the KVM accelerator and when running on a host CPU type that
> -supports the feature.
> +supports the feature.  While `aarch64` currently only works with KVM,
> +it could work with TCG.  CPU features that are specific to KVM are
> +prefixed with "kvm-" and are described in "KVM VCPU Features".
>
>  CPU Feature Probing
>  ===================
> @@ -171,6 +173,33 @@ disabling many SVE vector lengths would be quite 
> verbose, the `sve<N>` CPU
>  properties have special semantics (see "SVE CPU Property Parsing
>  Semantics").
>
> +KVM VCPU Features
> +=================
> +
> +KVM VCPU features are CPU features that are specific to KVM, such as
> +paravirt features or features that enable CPU virtualization extensions.
> +The features' CPU properties are only available when KVM is enabled and
> +are named with the prefix "kvm-".  KVM VCPU features may be probed,
> +enabled, and disabled in the same way as other CPU features.  Below is the
> +list of KVM VCPU features and their descriptions.
> +
> +  kvm-no-adjvtime          When disabled, each time the VM transitions
> +                           back to running state from the paused state the
> +                           VCPU's vitual counter is updated to ensure the

"virtual"

> +                           stopped time is not counted.  This avoids time
> +                           jumps surprising guest OSes and applications,
> +                           as long as they use the virtual counter for
> +                           timekeeping, but has the side effect of the
> +                           virtual and physical counters diverging.  All
> +                           timekeeping based on the virtual counter will
> +                           appear to lag behind any timekeeping that does
> +                           not subtract VM stopped time.  The guest may
> +                           resynchronize its virtual counter with other
> +                           time sources as needed.  Enabling this KVM VCPU
> +                           feature provides the legacy behavior, which is
> +                           to also count stopped time with the virtual
> +                           counter.

This phrasing reads a bit confusingly to me. What I would usually expect
is that you get
  name-of-option              Description of what the option does.

But here we have
  name-of-option              Long description of the default behaviour,
                              taking many lines and several sentences.
                              Brief note at the end that enabling this
                              feature gives the opposite effect.

Especially since the default-behaviour description isn't prefaced
with "By default" or similar, it's quite easy to start reading the
text assuming it's defining what the option is going to do, only
to get to the end and realise that it's defining what the option
is *not* going to do...

Incidentally, if I understand things correctly, for TCG the
behaviour is (and has always been) that VM-stopped time is
not counted, because we run the emulated versions of these counters
off QEMU_CLOCK_VIRTUAL. So having the KVM default be the same as
the TCG default is nicely consistent.

thanks
-- PMM