Re: [PATCH v15 09/13] tsc: Use the GUEST_TSC_FREQ MSR for discovering TSC frequency

["Nikunj A. Dadhania" <nikunj@amd.com>]

On 1/2/2025 4:15 PM, Borislav Petkov wrote:
> On Thu, Jan 02, 2025 at 03:31:49PM +0530, Nikunj A. Dadhania wrote:
>> Sure, how about the below:
>>
>> For SecureTSC enabled guests, TSC frequency should be obtained from the platform 
>> provided GUEST_TSC_FREQ MSR (C001_0134h). As paravirt clock(kvm-clock) has over-ridden 
>> x86_platform.calibrate_cpu() and x86_platform.calibrate_tsc() callbacks, 
>> SecureTSC needs to override them with its own callbacks.
> 
> Not really.
> 
> It's like you're in a contest of "how to write a commit message which is the
> shortest and has as less information as possible."

That is not the goal :-)

Patch 9

---------------------------------------------------------------------------
x86/tsc: Use the GUEST_TSC_FREQ MSR for discovering TSC frequency

Although the kernel switches over to stable TSC clocksource instead of
kvm-clock, TSC frequency calibration still keeps on using the kvm-clock
based frequency calibration. This is due to kvmclock_init() updating the
x86_platform's CPU and TSC callbacks unconditionally.

For SecureTSC enabled guests, use the GUEST_TSC_FREQ MSR to discover the
TSC frequency instead of relying on kvm-clock based frequency calibration.
Override both CPU and TSC frequency calibration callbacks with
securetsc_get_tsc_khz(). As difference between CPU base and TSC frequency
does not apply in this case same callback is being used.
---------------------------------------------------------------------------


> 
> Go back, read the subthread and summarize, please, what has been discussed
> here and for patch 12.

Here is the updated commit log for patch 12:

---------------------------------------------------------------------------
x86/kvmclock: Warn when kvmclock is selected for SecureTSC enabled guests

Warn users when kvmclock is selected as the clock source for SecureTSC enabled
guests. Users can change the clock source to kvm-clock using the sysfs interface
while running on a Secure TSC enabled guest. Switching to the hypervisor-controlled
kvmclock defeats the purpose of using SecureTSC.

Taint the kernel and issue a warning to the user when the clock source switches
to kvmclock, ensuring they are aware of the change and its implications.

---------------------------------------------------------------------------

 
> I'm missing the big picture about the relationship between kvmclock and TSC
> in STSC guests. 

kvmclock_init() always runs before tsc_early_init(). kvm-clock registers the
following during the initialization:

1) TSC calibration callbacks (addressed by patch 9)
2) sched clock (addressed by patch 11)
3) kvm-clock as the clocksource (addressed by patch 10)
4) wall clock callbacks (we don't have a solution for this yet)

I had a summary about this here [1], below snippet with slight modifications after review:

---
To summarise this thread with respect to TSC vs KVM clock, there three key questions:

1) When should kvmclock init be done?
2) How should the TSC frequency be discovered?
3) What should be the sched clock source and how should it be selected in a generic way?

○ Legacy CPU/VMs: VMs running on platforms without non-stop/constant TSC 
  + kvm-clock should be registered before tsc-early/tsc
  + Need to calibrate TSC frequency
  + Use kvmclock wallclock
  + Use kvmclock for sched clock

○ Modern CPU/VMs: VMs running on platforms supporting constant, non-stop and reliable TSC
  + kvm-clock should be registered before tsc-early/tsc
  + TSC Frequency:
      For SecureTSC: GUEST_TSC_FREQ MSR (C001_0134h) provides the TSC frequency, other 
      AMD guests need to calibrate the TSC frequency.
  + Use kvmclock wallclock
  + Use TSC for sched clock

----

> 
> After asking so many questions, it sounds to me like this patch and patch 12
> should be merged into one and there it should be explained what the strategy
> is when a STSC guest loads and how kvmclock is supposed to be handled, what is
> allowed, what is warned about, when the guest terminates what is tainted,
> yadda yadda. 
> > This all belongs in a single patch logically.



Regards
Nikunj
 
1: https://lore.kernel.org/lkml/64813123-e1e2-17e2-19e8-bd5c852b6a32@amd.com/