qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Jones <drjones@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: bijan.mottahedeh@oracle.com, Marc Zyngier <maz@kernel.org>,
	Richard Henderson <richard.henderson@linaro.org>,
	QEMU Developers <qemu-devel@nongnu.org>,
	qemu-arm <qemu-arm@nongnu.org>, Heyi Guo <guoheyi@huawei.com>,
	msys.mizuma@gmail.com
Subject: Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment
Date: Mon, 16 Dec 2019 17:36:04 +0100	[thread overview]
Message-ID: <20191216163604.wje5q2mvtytxjqoy@kamzik.brq.redhat.com> (raw)
In-Reply-To: <CAFEAcA_u94O8WYLgB8DF=pu-3V7LrNWpiQFV5mDYeeqLj1Ee2Q@mail.gmail.com>

On Mon, Dec 16, 2019 at 03:14:00PM +0000, Peter Maydell wrote:
> 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 ?

I don't think we should need to worry about this state, because migration
will do its own save/restore of the virtual counter, and as that restore
comes later, it'll take precedence. We still need this state for the usual
save/restore when not migrating, though, because KVM_REG_ARM_TIMER_CNT is
a non-runtime cpreg with its level set to KVM_PUT_FULL_STATE.

> 
> > +
> > +    /* 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()) ?

It will, but only when level == KVM_PUT_FULL_STATE.


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

Actually it doesn't really matter which takes precedence (I don't think),
which is why we can rely on the usual save/restore for migration. We only
need the new state this patch adds because we don't have any recent state
otherwise, and because then we can be selective and only do the
save/restore when transitioning to/from paused state.

Thanks,
drew



  parent reply	other threads:[~2019-12-16 17:15 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-12 17:33 [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time Andrew Jones
2019-12-12 17:33 ` [RFC PATCH v2 1/5] hw: add compat machines for 5.0 Andrew Jones
2019-12-12 18:27   ` David Hildenbrand
2019-12-12 19:24   ` Eduardo Habkost
2019-12-13  7:10     ` Andrew Jones
2019-12-13  5:00   ` David Gibson
2019-12-12 17:33 ` [RFC PATCH v2 2/5] target/arm/kvm64: kvm64 cpus have timer registers Andrew Jones
2019-12-12 17:33 ` [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment Andrew Jones
2019-12-16 15:14   ` Peter Maydell
2019-12-16 15:40     ` Peter Maydell
2019-12-16 16:43       ` Andrew Jones
2019-12-16 18:06         ` Peter Maydell
2019-12-19 14:30           ` Andrew Jones
2020-01-20  9:40             ` Andrew Jones
2019-12-16 16:36     ` Andrew Jones [this message]
2019-12-12 17:33 ` [RFC PATCH v2 4/5] tests/arm-cpu-features: Check feature default values Andrew Jones
2019-12-12 17:33 ` [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property Andrew Jones
2019-12-16 15:06   ` Peter Maydell
2019-12-16 16:52     ` Andrew Jones
2019-12-16 16:57       ` Peter Maydell
2020-01-20 10:31     ` Andrew Jones
2020-02-06 12:08   ` Philippe Mathieu-Daudé
2020-02-06 12:40     ` Andrew Jones
2020-02-06 22:46       ` Philippe Mathieu-Daudé
2020-02-07  7:37         ` Andrew Jones
2019-12-16 15:33 ` [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time Peter Maydell
2019-12-16 15:44   ` Peter Maydell
2020-01-20 13:45     ` Andrew Jones
2019-12-16 16:18   ` Marc Zyngier
2019-12-16 16:59     ` Andrew Jones
2019-12-16 17:05       ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191216163604.wje5q2mvtytxjqoy@kamzik.brq.redhat.com \
    --to=drjones@redhat.com \
    --cc=bijan.mottahedeh@oracle.com \
    --cc=guoheyi@huawei.com \
    --cc=maz@kernel.org \
    --cc=msys.mizuma@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).