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

[Andrew Jones <drjones@redhat.com>]

On Mon, Dec 16, 2019 at 03:06:57PM +0000, Peter Maydell wrote:
> On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> 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 <drjones@redhat.com>
> > ---
> >  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...

I'll take another stab at this, but my feeling is that a '-no-' option
should be one that just turns off the default behavior, which is why I
wrote a long description of the default behavior. If you'd prefer the
description to be more terse, then I can certainly delete a bunch of
the text, but then I fear what this option disables wouldn't be clear
enough.

> 
> 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,
drew