Re: [PATCH 00/10] KVM: Add idempotent controls for migrating system counter state

From: Christian Borntraeger <borntraeger@de.ibm.com>
To: Paolo Bonzini <pbonzini@redhat.com>, Oliver Upton <oupton@google.com>
Cc: kvm list <kvm@vger.kernel.org>,
	kvmarm@lists.cs.columbia.edu,
	Maxim Levitsky <mlevitsk@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Marc Zyngier <maz@kernel.org>, Peter Shier <pshier@google.com>,
	Jim Mattson <jmattson@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>
Subject: Re: [PATCH 00/10] KVM: Add idempotent controls for migrating system counter state
Date: Thu, 10 Jun 2021 08:53:43 +0200	[thread overview]
Message-ID: <a3754185-ad03-c20d-d3df-9ddfd0187c99@de.ibm.com> (raw)
In-Reply-To: <7b57ce79-6a17-70ac-4639-47a0df463e49@redhat.com>

On 10.06.21 08:22, Paolo Bonzini wrote:
> On 10/06/21 00:04, Oliver Upton wrote:
>>> Your approach still needs to use the "quirky" approach to host-initiated
>>> MSR_IA32_TSC_ADJUST writes, which write the MSR without affecting the
>>> VMCS offset.  This is just a documentation issue.
>>
>> My suggested ioctl for the vCPU will still exist, and it will still
>> affect the VMCS tsc offset, right? However, we need to do one of the
>> following:
>>
>> - Stash the guest's MSR_IA32_TSC_ADJUST value in the
>> kvm_system_counter_state structure. During
>> KVM_SET_SYSTEM_COUNTER_STATE, check to see if the field is valid. If
>> so, treat it as a dumb value (i.e. show it to the guest but don't fold
>> it into the offset).
> 
> Yes, it's already folded into the guestTSC-hostTSC offset that the caller provides.
> 
>> - Inform userspace that it must still migrate MSR_IA32_TSC_ADJUST, and
>> continue to our quirky behavior around host-initiated writes of the
>> MSR.
>>
>> This is why Maxim's spin migrated a value for IA32_TSC_ADJUST, right?
> 
> Yes, so that he could then remove (optionally) the quirk for host-initiated writes to the TSC and TSC_ADJUST MSRs.
> 
>> Doing so ensures we don't have any guest-observable consequences due
>> to our migration of TSC state. To me, adding the guest IA32_TSC_ADJUST
>> MSR into the new counter state structure is probably best. No strong
>> opinions in either direction on this point, though:)
> 
> Either is good for me, since documentation will be very important either way.  This is a complex API to use due to the possibility of skewed TSCs.
> 
> Just one thing, please don't introduce a new ioctl and use KVM_GET_DEVICE_ATTR/KVM_SET_DEVICE_ATTR/KVM_HAS_DEVICE_ATTR.
> 
> Christian, based on what Oliver mentions here, it's probably useful for s390 to have functionality to get/set kvm->arch.epoch and kvm->arch.epdx in addition to the absolute TOD values that you are migrating now.

Yes, a scheme where we migrate the offsets (assuming that the hosts are synced) would be often better.
If the hosts are not synced, things will be harder. I will have a look at this series, Thanks for the pointer.