Message-ID: <51dea3a632131d9a49af3991a633f26ce8592dd3.camel@infradead.org>
Date: Tue, 07 May 2024 20:08:40 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Marcelo Tosatti <mtosatti@...hat.com>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>, Jonathan
 Corbet <corbet@....net>, Sean Christopherson <seanjc@...gle.com>, Thomas
 Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav
 Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, Paul Durrant
 <paul@....org>, Shuah Khan <shuah@...nel.org>,  linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org,  linux-kselftest@...r.kernel.org, Oliver
 Upton <oliver.upton@...ux.dev>,  jalliste@...zon.co.uk, sveith@...zon.de,
 zide.chen@...el.com, Dongli Zhang <dongli.zhang@...cle.com>
Subject: Re: [PATCH v2 01/15] KVM: x86/xen: Do not corrupt KVM clock in
 kvm_xen_shared_info_init()

On Sat, 2024-05-04 at 04:42 -0300, Marcelo Tosatti wrote:
> On Sat, Apr 27, 2024 at 12:04:58PM +0100, David Woodhouse wrote:
> > 
> > In particular, KVM_REQ_MASTERCLOCK_UPDATE will take a new snapshot of
> > time as the reference in master_kernel_ns and master_cycle_now, yanking
> > the guest's clock back to match definition A at that moment.
> 
> KVM_REQ_MASTERCLOCK_UPDATE stops the vcpus because:

Took me a while to work that one out, btw. The fact that the 
KVM_REQ_MCLOCK_INPROGRESS request is asserted but never actually
*handled*, so all it does is repeatedly kick the vCPU out and make it
spin until the request is cleared is... interesting. Likewise the way
that we set KVM_REQ_MASTERCLOCK_UPDATE on *all* vCPUs, so they *all*
call kvm_update_masterclock(), when only one of them needed to. I may
clean that up a little.

>  * To avoid that problem, do not allow visibility of distinct
>  * system_timestamp/tsc_timestamp values simultaneously: use a master
>  * copy of host monotonic time values. Update that master copy
>  * in lockstep.

Right. That comment is a lot longer than the part you cited here, and
starts with 'assuming a stable TSC across pCPUS, and a stable TSC
across vCPUs'. It's the "if (ka->use_master_clock)" case.

And yes, what it's basically saying is a special case of the fact that
if you let the KVM clock run at its "natural" rate based on the guest
TSC (definition B), but each vCPU runs at that rate from a *different*
point on the line that is definition A (the host CLOCK_MONOTONIC_RAW),
bad things will happen.

I'm OK with it stopping the vCPUs (although I'd like it to do so in a
less implicitly awful way). But when we don't need to update the
reference time at all, let's not do so.

> > When invoked from in 'use_master_clock' mode, kvm_update_masterclock()
> > should probably *adjust* kvm->arch.kvmclock_offset to account for the
> > drift, instead of yanking the clock back to defintion A.
> 
> You are likely correct...
> 
> > But in the meantime there are a bunch of places where it just doesn't need to be
> > invoked at all.
> > 
> > To start with: there is no need to do such an update when a Xen guest
> > populates the shared_info page. This seems to have been a hangover from
> > the very first implementation of shared_info which automatically
> > populated the vcpu_info structures at their default locations, but even
> > then it should just have raised KVM_REQ_CLOCK_UPDATE on each vCPU
> > instead of using KVM_REQ_MASTERCLOCK_UPDATE. And now that userspace is
> > expected to explicitly set the vcpu_info even in its default locations,
> > there's not even any need for that either.
> > 
> > Fixes: 629b5348841a1 ("KVM: x86/xen: update wallclock region")
> > Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
> > Reviewed-by: Paul Durrant <paul@....org>
> > ---
> >  arch/x86/kvm/xen.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index f65b35a05d91..5a83a8154b79 100644
> > --- a/arch/x86/kvm/xen.c
> > +++ b/arch/x86/kvm/xen.c
> > @@ -98,8 +98,6 @@ static int kvm_xen_shared_info_init(struct kvm *kvm)
> >         wc->version = wc_version + 1;
> >         read_unlock_irq(&gpc->lock);
> >  
> > -       kvm_make_all_cpus_request(kvm, KVM_REQ_MASTERCLOCK_UPDATE);
> > -
> >  out:
> >         srcu_read_unlock(&kvm->srcu, idx);
> >         return ret;
> > -- 
> > 2.44.0
> 
> So KVM_REQ_MASTERCLOCK_UPDATE is to avoid the race above.
> 
> In what contexes is kvm_xen_shared_info_init called from again?
> 
> Not clear to me KVM_REQ_MASTERCLOCK_UPDATE is not needed (or that is
> needed, for that matter...).

We cal kvm_xen_shared_info_init() when the Xen "shared info" page is
set up. The only interesting part of that is the *wallclock* epoch.

The wallclock (just like KSR_KVM_WALL_CLOCK{,_NEW}) is *entirely* hosed
ever since the KVM clock stopped being based on CLOCK_MONOTONIC, since
that means that the value of "wallclock time minus KVM clock time"
actually *changes* as the KVM clock runs at a different rate to
wallclock time. 

I'm looking at a replacement for that which uses the gtod information
to give the guest a direct mapping of guest TSC to host CLOCK_TAI. And
in doing so we can *also* indicate when live migration has potentially
disrupted the guest TSC, so any further NTP/PTP refinement that the
guest may have done for itself needs to be thrown away.


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)

Powered by blists - more mailing lists