Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Tue, 31 Oct 2023 11:42:19 +0000
From:   David Woodhouse <dwmw2@infradead.org>
To:     paul@xen.org, Paul Durrant <xadimgnik@gmail.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
CC:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH] KVM: x86/xen: improve accuracy of Xen timers
User-Agent: K-9 Mail for Android
In-Reply-To: <1a679274-bbff-4549-a1ea-c7ea9f1707cc@xen.org>
References: <96da7273adfff2a346de9a4a27ce064f6fe0d0a1.camel@infradead.org> <1a679274-bbff-4549-a1ea-c7ea9f1707cc@xen.org>
Message-ID: <F80266DD-D7EF-4A26-B9F8-BC33EC65F444@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On 31 October 2023 10:42:42 GMT, Paul Durrant <xadimgnik@gmail=2Ecom> wrot=
e:
>There is no documented ordering requirement on setting KVM_XEN_VCPU_ATTR_=
TYPE_TIMER versus KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO or KVM_XEN_ATTR_TYPE_SHA=
RED_INFO but kvm_xen_start_timer() now needs the vCPU's pvclock to be valid=
=2E Should actually starting the timer not be deferred until then? (Or simp=
ly add a check here and have the attribute setting fail if the pvclock is n=
ot valid)=2E


There are no such dependencies and I don't want there to be=2E That would =
be the *epitome* of what my "if it needs documenting, fix it first" mantra =
is intended to correct=2E

The fact that this broke on migration because the hv_clock isn't set up ye=
t, as we saw in our overnight testing, is just a bug=2E In my tree I've fix=
ed it thus:

index 63531173dad1=2E=2Ee3d2d63eef34 100644
--- a/arch/x86/kvm/xen=2Ec
+++ b/arch/x86/kvm/xen=2Ec
@@ -182,7 +182,7 @@ static void kvm_xen_start_timer(st
ruct kvm_vcpu *vcpu, u64 guest_abs,
         * the absolute CLOCK_MONOTONIC time at which
the timer should
         * fire=2E
         */
-       if (vcpu->kvm->arch=2Euse_master_clock &&
+       if (vcpu->arch=2Ehv_clock=2Eversion && vcpu->kvm->
arch=2Euse_master_clock &&
            static_cpu_has(X86_FEATURE_CONSTANT_TSC))
{
                uint64_t host_tsc, guest_tsc;

@@ -206,9 +206,23 @@ static void kvm_xen_start_timer(s
truct kvm_vcpu *vcpu, u64 guest_abs,

                /* Calculate the guest kvmclock as the
 guest would do it=2E */
                guest_tsc =3D kvm_read_l1_tsc(vcpu, host
_tsc);
-               guest_now =3D __pvclock_read_cycles(&vcp
u->arch=2Ehv_clock, guest_tsc);
+               guest_now =3D __pvclock_read_cycles(&vcp
u->arch=2Ehv_clock,
+                                                 gues
t_tsc);
        } else {
-               /* Without CONSTANT_TSC, get_kvmclock_
ns() is the only option */
+               /*
+                * Without CONSTANT_TSC, get_kvmclock_
ns() is the only option=2E
+                *
+                * Also if the guest PV clock hasn't b
een set up yet, as is
+                * likely to be the case during migrat
ion when the vCPU has
+                * not been run yet=2E It would be possi
ble to calculate the
+                * scaling factors properly in that ca
se but there's not much
+                * point in doing so=2E The get_kvmclock
_ns() drift accumulates
+                * over time, so it's OK to use it at
startup=2E Besides, on
+                * migration there's going to be a lit
tle bit of skew in the
+                * precise moment at which timers fire
 anyway=2E Often they'll
+                * be in the "past" by the time the VM
 is running again after
+                * migration=2E
+                */
                guest_now =3D get_kvmclock_ns(vcpu->kvm)
;
                kernel_now =3D ktime_get();
        }
--
2=2E41=2E0

We *could* reset the timer when the vCPU starts to run and handles the KVM=
_REQ_CLOCK_UPDATE event, but I don't want to for two reasons=2E

Firstly, we just don't need that complexity=2E This approach is OK, as the=
 newly-added comment says=2E And we do need to fix get_kvmclock_ns() anyway=
, so it should work fine=2E Most of this patch will still be useful as it u=
ses a single TSC read and we *do* need to do that part even after all the k=
vmclock brokenness is fixed=2E But the complexity on KVM_REQ_CLOCK_UPDATE i=
sn't needed in the long term=2E

Secondly, it's also wrong thing to do in the general case=2E Let's say KVM=
 does its thing and snaps the kvmclock backwards in time on a KVM_REQ_CLOCK=
_UPDATE=2E=2E=2E do we really want to reinterpret existing timers against t=
he new kvmclock? They were best left alone, I think=2E=20
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=00=
=00=00=00=00=00