Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <fa081a65-6abd-c8dc-240c-b25d6af55bb2@oracle.com>
Date:   Tue, 17 Oct 2023 09:18:41 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock
 periodically
To:     Sean Christopherson <seanjc@google.com>
Cc:     David Woodhouse <dwmw2@infradead.org>,
        Joe Jin <joe.jin@oracle.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de, dave.hansen@linux.intel.com
References: <ZSXqZOgLYkwLRWLO@google.com>
 <8f3493ca4c0e726d5c3876bb7dd2cfc432d9deaa.camel@infradead.org>
 <ZSmHcECyt5PdZyIZ@google.com>
 <cf2b22fc-78f5-dfb9-f0e6-5c4059a970a2@oracle.com>
 <ZSnSNVankCAlHIhI@google.com>
 <BD4C4E71-C743-4B79-93CA-0F3AC5423412@infradead.org>
 <993cc7f9-a134-8086-3410-b915fe5db7a5@oracle.com>
 <03afed7eb3c1e5f4b2b8ecfd8616ae5c6f1819e9.camel@infradead.org>
 <ZS2Fq5dr2CeZaBok@google.com>
 <b3721f33-d5b0-79db-8500-d6b93dded0c1@oracle.com>
 <ZS29teniU3xer0Xu@google.com>
Content-Language: en-US
From:   Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <ZS29teniU3xer0Xu@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Nlp6ZUFNcTdIWkwxdlc4OG5RZEQzb3pYMTBUSlM0VGdSNkhXamRESzdSQjJz?=
 =?utf-8?B?Tlp6YlFTQVpsTVN0dXduUXVwVHlKTWdDTnNUQTkrMWxGcFhUVUt0UGQ2d1pu?=
 =?utf-8?B?cU1JNFZqejNxVmdCcnJuL21YTUV1WUVBdGZVQUxzWmRUN0dqZ2NnalVVeWty?=
 =?utf-8?B?UGFNbmhrdVhhd21LWHZhN25BVDArSUdZUzhBeGZxeWpnMHNFRWdGTEpBNEl5?=
 =?utf-8?B?MGo1UzVBMW5SOUhRSHZxVkV0dkVaWGNQVjB3Vks3YkgxT09qOTladGMwWE84?=
 =?utf-8?B?S2YvTVM3azcwWW0rN25aaW1NOTM3VEdpUlBVUGlqWC9CY2g1TmU1SWd5TjZN?=
 =?utf-8?B?WmI4M2lEUDVPKzBPbFJIdldjQVpUbnRsemdxdHRRREIxUG10WU9iY2NRWi90?=
 =?utf-8?B?ZGc3Y2hzQ0IyQ1RBNGlSVkU2a3BweWw4Z211ZVZvNEhNZW1iampzeXVWMUs1?=
 =?utf-8?B?ekhqU05CQUtpV3ZHTnkvV3hPbjZydXhOamdYMFhiSCtTY1J3cUxPS2JWOXFs?=
 =?utf-8?B?V3M3SEhaQ1ZBaVRuRlFxU0hsSHJrdnBwWmtYRDg1bE5seVhYK3dwNHE2dHYz?=
 =?utf-8?B?VnVvaGhTYk1aOHBmWkhWY1hLV0tXNFB0S3ZQRmlMTTB4Nlk3QTFyaG1CVzlh?=
 =?utf-8?B?QVRWYy9aY0tLbkl6NmVESHB6dmRsM0lpTWhybXlSMCtDVmM3ZElPZ2FsKzRQ?=
 =?utf-8?B?ZzF1Ulh3NEFyZVpaS01yTFlGdUZobW91eTZPSU1lQzdHMldIb2lSekQ4V1RL?=
 =?utf-8?B?T2JFL1gzdzhuUlhnSW5GZ2FybWVJOE9UZndDNzY3TTFLRWJEVXVzakIzRFI2?=
 =?utf-8?B?Z1lpN2NhZkI0L2g1V1NvZjFaU3FjVVZuUGxPcWttU3R3N0pBaDZ4cXZDb3ZJ?=
 =?utf-8?B?aDhUWTkzMzk1S1Rkczg1blpFYVE2RjIvMVplU0krS2VMVW5lTy9meVd6bHcr?=
 =?utf-8?B?Vk9VV2R3Z2dYckg0NzNsYVhFUlBsWi90RlR5cHJvSUpFZ0NpenZIRFBONzlH?=
 =?utf-8?B?aEs5WjB1R092YUp1QitRNjR3MGZ5TkgzNWF2UHpFZGM4VXpTQTBic0JuUlJK?=
 =?utf-8?B?UkZwc01zNThDTkdZd1YycFE1L1BpTDd2d1ZWelhadWdYSnU3a2RZS3F1YUQx?=
 =?utf-8?B?VnVYbEtEQU9nVGlmc0xvY3E3T3RBbS9BSU9nZzREZXdwamt2VnYxek9lMzZx?=
 =?utf-8?B?NHVSbHlMdXhqdExXM3UvcjkzYmMwY2I3MGV1T1VOVXlFY2Z0eml1MGhWOFVJ?=
 =?utf-8?B?UjNaVCtjMEZzUjhtQU8rQU1sZEh1V1l5YTJSajJJK1JuODU0T0lacU5wWnBT?=
 =?utf-8?B?UjBHRjZkK25JY1o3U2Y5VWtjZ1RNUlZlWU9TUG45VHhPSnkyd0VoNVdmSG5C?=
 =?utf-8?B?ZWZhZ0N3WkxobWQwMGFLandCTGkrQ25qTVZlWTkrM3VIeVJvREN3SndtNDVj?=
 =?utf-8?B?cWgrd3BWR3orR1VpR0wzTkRCaitIbTVPZG1RK3RycG45aVh3Q1plL01BbmVD?=
 =?utf-8?B?M2FuVGVESVBIcXEwMzdBZzhwYWVBRUpRQ0E3Z0JObytsV2J5Y2ZWTG9hZFNF?=
 =?utf-8?B?bjVnUm9XTS9IZFlkNmRNc1lRNkJFbWhiMk96TVp1NmczdWNpT3hXaTdjYXVT?=
 =?utf-8?B?QVIwSEN0YTVieFNrMmJPTmRvTVRKMTNDNUVRbXRKK1N1a1VyYXpWZHdKTHRX?=
 =?utf-8?B?VjRQTTJzcVJwRFlhaXF2bXB6dUc1VXpQS24yaDJBV3hXYlBQbUo4WU5FN0d0?=
 =?utf-8?B?bVZhQzRjZTZrL3F4UTJuUmNiKzlIdTZhUzdSOXhTcytiWDFnZ3EyY2ZDcCtj?=
 =?utf-8?B?V2t6d092cy9qSHl2cU1Ma0VLTGlHR0JCQ0E2bFdDa0gxb2ZNelBmT25sdkFp?=
 =?utf-8?B?SDFRQitIZDQrbUtXNGxQVjJQTlVIZWlsa0tlYnhWSTNkL250WWdvbkw4NG5U?=
 =?utf-8?B?ZGh6L3FSYWxyWWVGTDlrODAybUNaWFpubnlyQzl3UTRTY3ZBcllkZFV2N1p2?=
 =?utf-8?B?WlJZWU5OWEtGODRVMFZXNVJlNThjVkpEM09tSTE5ckhpU0QxZ1lXTE5YV2xr?=
 =?utf-8?B?b3dORjY4QWI3aHdDT0kzRXhSd0NZbUZWMGhKRGl1SWIxZ3RQRllNcmhudXNN?=
 =?utf-8?Q?fvyYxUtNMj0yeyEsO8NHT6kec?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?YnZNb1REaFNpSWNjK2hPK1lBMWRNVG10aVhLc0NXbUtSUEJGZHR4dG5xRTl4?=
 =?utf-8?B?RGswSFBjU21IbUJMMFd5Ujlrdk1ocWJHUHByUWdlWWFEdzYwRmFqU1FweFRZ?=
 =?utf-8?B?TFF0WkRKY1pRMkhTS0ZXM21tTkZnTzlNTnIxTWFUa3JCNnd0UkxITmV0R1Rl?=
 =?utf-8?B?SXErTmFBN1ZuSmQvUWFQd0Q1Q2Qzd0VuZWRXTFUySDR0QUdJR0RTTjdRVGVx?=
 =?utf-8?B?NFkzVEdnRTgvbG43dGlyUkFqY0h1RUN5d3FUVEpyMHcvTUZhVnBpTDNJNVpx?=
 =?utf-8?B?aVk5QzBvMzJMNUtReE1ZQjdDVUozY1c0QXd3ZGg3KzNuWS9QbnRsZVRodUpT?=
 =?utf-8?B?bXYzRm9wSCtZRDcweklhUURDaFZMemd4b2prNXBtSFJYM212RnNLT2E2eE5S?=
 =?utf-8?B?eURZNFYyZ3dTL3Fvb1FwVWdyWXhNNTBTWUlMSGRjVlN5Y25GNjgvME5QYVVr?=
 =?utf-8?B?Q0lRcDFjU0ZhL0JKOXp2MWNBRUtwQjRyNUliU2pYMjhPVEVvM09nS1VtQ0ZV?=
 =?utf-8?B?NzJlQmdzejNKWHllTTUvNCtDdzZsTUZZa3pmVm1DY0srelM0QlBsNWZSUDdW?=
 =?utf-8?B?RjJVVWNqcTJzZ0xxemtBMDVBaE5LdGcrV0duUlNWY2s3M2hvci9YTUJpeENJ?=
 =?utf-8?B?bit1YlAvaGVYN2R3dVdTMWwwYlhldWpMVGtxbEZIRXAvY0t2OWtEb2habGo4?=
 =?utf-8?B?OVRheWthbGhtM0xFQTZYM2trWitEWkpWT1IzcVc2UjdRRTd3ZmY0bDJ1OUNz?=
 =?utf-8?B?d1lyTnVidXNnL2IzMGVWT2d4dkRXK01IRytKYTVRdzJ2YWdlM1Y0RFlzbVU5?=
 =?utf-8?B?RFFCcEtqRytyMHhyOHpUd0N5ZGVrM3picEg4ekFKc0MxdktzRWZNSHFIY0xy?=
 =?utf-8?B?ZmVFMGlRSHpRSDB3d01SRmppYjBQWSt3dWZuNGZSTjdJWnBPcUFVYXlUcC9o?=
 =?utf-8?B?UjlpRCt2WjdlQUt2UlRVUDI5V0lzUDhPYWxJaTFwbWVrdkR1bEU1UGFYZFhG?=
 =?utf-8?B?L0pmN3cvakxRRmpSMDN0TUJOZThHWjNsVzRDaXE0NmxEUWhxR0JST2p2UDJa?=
 =?utf-8?B?Mm5Lc0E4cHZSRG14ZUFud1BSdjBnbnh2am9zL2x0eUljalE1cnJwblFjbndU?=
 =?utf-8?B?cmZzRnAvVlo1K2ZPeE5VY0xldzhRdytjUlZCbEdUVEI3Y29JdXVGendTeUxV?=
 =?utf-8?B?aGl0YWxlZ2xXQnRqb3VHVFhMOWhmUGFYdEhNV2dGMXNNUkZNQWVBVjdrK1hi?=
 =?utf-8?B?QUdORjNuZlE0c0t6ekxlanRrek50cy96SWk1L25UQWo1VG12UFJzeENTM3Vz?=
 =?utf-8?Q?OxfkgglJTZCaEuMVnuIB4R+a2AnsRvuvVA?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9b25beab-b74a-407d-516d-08dbcf2cc052
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Oct 2023 16:18:51.8210
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1SAG6qAdD19FdO0XNHUPnkajHR0pBskI/KOjha954V6yAQW3Y8/ivQc43Zi/j3+7hshZfq/592wyJTvZDLjn/A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB7292
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-17_03,2023-10-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 spamscore=0
 mlxscore=0 phishscore=0 bulkscore=0 suspectscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2310170137
X-Proofpoint-GUID: WZ65fpXn_wvttbKozWeU8REGkJieuDlM
X-Proofpoint-ORIG-GUID: WZ65fpXn_wvttbKozWeU8REGkJieuDlM
X-Spam-Status: No, score=-4.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 17 Oct 2023 09:19:51 -0700 (PDT)

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