Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically

[Dongli Zhang]

Hi Sean,

On 10/16/23 15:48, Sean Christopherson wrote:
> On Mon, Oct 16, 2023, Dongli Zhang wrote:
>> Hi Sean,
>>
>> On 10/16/23 11:49, Sean Christopherson wrote:
>>> Compile tested only, but the below should fix the vCPU hotplug case.  Then
>>> someone (not me) just needs to figure out why kvm_xen_shared_info_init() forces
>>> a masterclock update.
>>>
>>> I still think we should clean up the periodic sync code, but I don't think we
>>> need to periodically sync the masterclock.
>>
>> This looks good to me. The core idea is to not update master clock for the
>> synchronized cases.
>>
>>
>> How about the negative value case? I see in the linux code it is still there?
> 
> See below.  
> 
>> (It is out of the scope of my expectation as I do not need to run vCPUs in
>> different tsc freq as host)
>>
>> Thank you very much!
>>
>> Dongli Zhang
>>
>>>
>>> ---
>>>  arch/x86/kvm/x86.c | 29 ++++++++++++++++-------------
>>>  1 file changed, 16 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index c54e1133e0d3..f0a607b6fc31 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -2510,26 +2510,29 @@ static inline int gtod_is_based_on_tsc(int mode)
>>>  }
>>>  #endif
>>>  
>>> -static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
>>> +static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu, bool new_generation)
>>>  {
>>>  #ifdef CONFIG_X86_64
>>> -	bool vcpus_matched;
>>>  	struct kvm_arch *ka = &vcpu->kvm->arch;
>>>  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
>>>  
>>> -	vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
>>> -			 atomic_read(&vcpu->kvm->online_vcpus));
>>> +	/*
>>> +	 * To use the masterclock, the host clocksource must be based on TSC
>>> +	 * and all vCPUs must have matching TSCs.  Note, the count for matching
>>> +	 * vCPUs doesn't include the reference vCPU, hence "+1".
>>> +	 */
>>> +	bool use_master_clock = (ka->nr_vcpus_matched_tsc + 1 ==
>>> +				 atomic_read(&vcpu->kvm->online_vcpus)) &&
>>> +				gtod_is_based_on_tsc(gtod->clock.vclock_mode);
>>>  
>>>  	/*
>>> -	 * Once the masterclock is enabled, always perform request in
>>> -	 * order to update it.
>>> -	 *
>>> -	 * In order to enable masterclock, the host clocksource must be TSC
>>> -	 * and the vcpus need to have matched TSCs.  When that happens,
>>> -	 * perform request to enable masterclock.
>>> +	 * Request a masterclock update if the masterclock needs to be toggled
>>> +	 * on/off, or when starting a new generation and the masterclock is
>>> +	 * enabled (compute_guest_tsc() requires the masterclock snaphot to be
>>> +	 * taken _after_ the new generation is created).
>>>  	 */
>>> -	if (ka->use_master_clock ||
>>> -	    (gtod_is_based_on_tsc(gtod->clock.vclock_mode) && vcpus_matched))
>>> +	if ((ka->use_master_clock && new_generation) ||
>>> +	    (ka->use_master_clock != use_master_clock))
>>>  		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>>>  
>>>  	trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc,
>>> @@ -2706,7 +2709,7 @@ static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc,
>>>  	vcpu->arch.this_tsc_nsec = kvm->arch.cur_tsc_nsec;
>>>  	vcpu->arch.this_tsc_write = kvm->arch.cur_tsc_write;
>>>  
>>> -	kvm_track_tsc_matching(vcpu);
>>> +	kvm_track_tsc_matching(vcpu, !matched);
> 
> If my analysis of how the negative timestamp occurred is correct, the problematic
> scenario was if cur_tsc_nsec/cur_tsc_write were updated without a masterclock update.
> Passing !matched for @new_generation means that KVM will force a masterclock update
> if cur_tsc_nsec/cur_tsc_write are changed, i.e. prevent the negative timestamp bug.


Thank you very much for the explanation. Now I understand it.

Thanks to the immediate call to kvm_synchronize_tsc() during each vCPU creation ...

kvm_vm_ioctl(KVM_CREATE_VCPU)
-> kvm_vm_ioctl_create_vcpu()
   -> kvm_arch_vcpu_postcreate()
      -> kvm_synchronize_tsc()

... the local variable "bool use_master_clock" in your patch may always be true.
At that time, the "(ka->use_master_clock != use_master_clock)" returns true.


As a result, we will be able to trigger the KVM_REQ_MASTERCLOCK_UPDATE  during
VM creation for each vCPU.

There is still KVM_REQ_MASTERCLOCK_UPDATE for each vCPU during VM creation.
However, there will be no KVM_REQ_MASTERCLOCK_UPDATE for vCPU hot-add.

Thank you very much!

Dongli Zhang