Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   Paul Durrant <xadimgnik@gmail.com>
Message-ID: <6c9671b4-d997-42ac-9821-06accb97357f@xen.org>
Date:   Tue, 31 Oct 2023 12:06:17 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Reply-To: paul@xen.org
Subject: Re: [PATCH] KVM: x86/xen: improve accuracy of Xen timers
Content-Language: en-US
To:     David Woodhouse <dwmw2@infradead.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>
References: <96da7273adfff2a346de9a4a27ce064f6fe0d0a1.camel@infradead.org>
 <1a679274-bbff-4549-a1ea-c7ea9f1707cc@xen.org>
 <F80266DD-D7EF-4A26-B9F8-BC33EC65F444@infradead.org>
Organization: Xen Project
In-Reply-To: <F80266DD-D7EF-4A26-B9F8-BC33EC65F444@infradead.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 31/10/2023 11:42, David Woodhouse wrote:
> 
> 
> On 31 October 2023 10:42:42 GMT, Paul Durrant <xadimgnik@gmail.com> wrote:
>> 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_SHARED_INFO but kvm_xen_start_timer() now needs the vCPU's pvclock to be valid. Should actually starting the timer not be deferred until then? (Or simply add a check here and have the attribute setting fail if the pvclock is not valid).
> 
> 
> There are no such dependencies and I don't want there to be. That would be the *epitome* of what my "if it needs documenting, fix it first" mantra is intended to correct.
> 
> The fact that this broke on migration because the hv_clock isn't set up yet, as we saw in our overnight testing, is just a bug. In my tree I've fixed it thus:
> 
> index 63531173dad1..e3d2d63eef34 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -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.
>           */
> -       if (vcpu->kvm->arch.use_master_clock &&
> +       if (vcpu->arch.hv_clock.version && vcpu->kvm->
> arch.use_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. */
>                  guest_tsc = kvm_read_l1_tsc(vcpu, host
> _tsc);
> -               guest_now = __pvclock_read_cycles(&vcp
> u->arch.hv_clock, guest_tsc);
> +               guest_now = __pvclock_read_cycles(&vcp
> u->arch.hv_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.
> +                *
> +                * 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. It would be possi
> ble to calculate the
> +                * scaling factors properly in that ca
> se but there's not much
> +                * point in doing so. The get_kvmclock
> _ns() drift accumulates
> +                * over time, so it's OK to use it at
> startup. Besides, on
> +                * migration there's going to be a lit
> tle bit of skew in the
> +                * precise moment at which timers fire
>   anyway. Often they'll
> +                * be in the "past" by the time the VM
>   is running again after
> +                * migration.
> +                */
>                  guest_now = get_kvmclock_ns(vcpu->kvm)
> ;
>                  kernel_now = ktime_get();
>          }
> --
> 2.41.0
> 
> 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.
> 
> Firstly, we just don't need that complexity. This approach is OK, as the newly-added comment says. And we do need to fix get_kvmclock_ns() anyway, so it should work fine. Most of this patch will still be useful as it uses a single TSC read and we *do* need to do that part even after all the kvmclock brokenness is fixed. But the complexity on KVM_REQ_CLOCK_UPDATE isn't needed in the long term.
> 
> Secondly, it's also wrong thing to do in the general case. Let's say KVM does its thing and snaps the kvmclock backwards in time on a KVM_REQ_CLOCK_UPDATE... do we really want to reinterpret existing timers against the new kvmclock? They were best left alone, I think.

Do we not want to do exactly that? If the master clock is changed, why 
would we not want to re-interpret the guest's idea of time? That update 
will be visible to the guest when it re-reads the PV clock source.

   Paul