Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <ee06976738dff35e387077ba73e6ab375963abbf.camel@redhat.com>
Subject: Re: [PATCH 1/2] KVM: x86: implement
 KVM_SET_TSC_PRECISE/KVM_GET_TSC_PRECISE
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org
Cc:     Oliver Upton <oupton@google.com>, Ingo Molnar <mingo@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        open list <linux-kernel@vger.kernel.org>,
        Marcelo Tosatti <mtosatti@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Wanpeng Li <wanpengli@tencent.com>,
        Borislav Petkov <bp@alien8.de>,
        Jim Mattson <jmattson@google.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        Joerg Roedel <joro@8bytes.org>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>
Date:   Mon, 30 Nov 2020 17:58:54 +0200
In-Reply-To: <38602ef4-7ecf-a5fd-6db9-db86e8e974e4@redhat.com>
References: <20201130133559.233242-1-mlevitsk@redhat.com>
         <20201130133559.233242-2-mlevitsk@redhat.com>
         <38602ef4-7ecf-a5fd-6db9-db86e8e974e4@redhat.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.3 (3.36.3-1.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Mon, 2020-11-30 at 15:33 +0100, Paolo Bonzini wrote:
> On 30/11/20 14:35, Maxim Levitsky wrote:
> > +		if (guest_cpuid_has(vcpu, X86_FEATURE_TSC_ADJUST)) {
> > +			tsc_state.tsc_adjust = vcpu->arch.ia32_tsc_adjust_msr;
> > +			tsc_state.flags |= KVM_TSC_STATE_TSC_ADJUST_VALID;
> > +		}
> 
> This is mostly useful for userspace that doesn't disable the quirk, right?

Isn't this the opposite? If I understand the original proposal correctly,
the reason that we include the TSC_ADJUST in the new ioctl, is that
we would like to disable the special kvm behavior (that is disable the quirk),
which would mean that tsc will jump on regular host initiated TSC_ADJUST write.

To avoid this, userspace would set TSC_ADJUST through this new interface.

Note that I haven't yet disabled the quirk in the patches I posted to the qemu,
because we need some infrastructure to manage which quirks we want to disable
in qemu
(That is, KVM_ENABLE_CAP is as I understand write only, so I can't just disable
KVM_X86_QUIRK_TSC_HOST_ACCESS, in the code that enables x-precise-tsc in qemu).

> 
> > +		kvm_get_walltime(&wall_nsec, &host_tsc);
> > +		diff = wall_nsec - tsc_state.nsec;
> > +
> > +		if (diff < 0 || tsc_state.nsec == 0)
> > +			diff = 0;
> > +
> 
> diff < 0 should be okay.  Also why the nsec==0 special case?  What about 
> using a flag instead?

In theory diff < 0 should indeed be okay (though this would mean that target,
has unsynchronized clock or time travel happened).

However for example nsec_to_cycles takes unsigned number, and then
pvclock_scale_delta also takes unsigned number, and so on,
so I was thinking why bother with this case.

There is still (mostly?) theoretical issue, if on some vcpus 'diff' is positive 
and on some is negative
(this can happen if the migration was really fast, and target has the clock
   A. that is only slightly ahead of the source).
Do you think that this is an issue? If so I can make the code work with
signed numbers.

About nsec == 0, this is to allow to use this API for VM initialization.
(That is to call KVM_SET_TSC_PRECISE prior to doing KVM_GET_TSC_PRECISE)

This simplifies qemu code, and I don't think 
that this makes the API much worse.

Best regards,
	Maxim Levitsky

> 
> Paolo
>