Re: [PATCH v11 19/20] x86/kvmclock: Skip kvmclock when Secure TSC is available

[Nikunj A. Dadhania]

On 10/1/2024 2:50 AM, Thomas Gleixner wrote:
> On Mon, Sep 30 2024 at 11:57, Nikunj A. Dadhania wrote:
>> TSC Clock Rating Adjustment:
>> * During TSC initialization, downgrade the TSC clock rating to 200 if TSC is not
>>   constant/reliable, placing it below HPET.
> 
> Downgrading a constant TSC is a bad idea. Reliable just means that it
> does not need a watchdog clocksource. If it's non-constant it's
> downgraded anyway.
> 
>> * Ensure the kvm-clock rating is set to 299 by default in the 
>>   struct clocksource kvm_clock.
>> * Avoid changing the kvm clock rating based on the availability of reliable
>>   clock sources. Let the TSC clock source determine and downgrade itself.
> 
> Why downgrade? If it's the best one you want to upgrade it so it's
> preferred over the others.

Thanks for confirming that upgrading the TSC rating is fine.

> The above will make sure that the PV clocksource rating remain
>> unaffected.
>>
>> Clock soure selection order when the ratings match:
>> * Currently, clocks are registered and enqueued based on their rating.
>> * When clock ratings are tied, use the advertised clock frequency(freq_khz) as a
>>   secondary key to favor clocks with better frequency.
>>
>> This approach improves the selection process by considering both rating and
>> frequency. Something like below:
> 
> What does the frequency tell us? Not really anything. It's not
> necessarily the better clocksource.
> 
> Higher frequency gives you a slightly better resolution, but as all of
> this is usually sub-nanosecond resolution already that's not making a
> difference in practice.
> 
> So if you know you want TSC to be selected, then upgrade the rating of
> both the early and the regular TSC clocksource and be done with it.

Sure Thomas, I will modify the patch accordingly and send an RFC.

Also I realized that, for the guests, instead of rdtsc(), we should be 
calling rdtsc_ordered() to make sure that time moves forward even when
vCPUs are migrated.

Thanks,
Nikunj