Re: [PATCH net] ptp: kvm: report paired cycles for the active clocksource

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] On SEV-SNP guests with Secure TSC, the new CSID_X86_TSC branch
  in kvm_arch_ptp_get_crosststamp() (drivers/ptp/ptp_kvm_x86.c) reports…

--

ptp: kvm: report paired cycles for the active clocksource

With this patch, kvm_arch_ptp_get_crosststamp() returns the raw paired TSC
value from the KVM_HC_CLOCK_PAIRING hypercall when CSID_X86_TSC is
requested. Before, it always converted that value to kvm-clock units. When
get_device_system_crosststamp() fails with -ENODEV,
ptp_kvm_getcrosststamp() now retries once with the active clocksource and
caches it in kvm_ptp_cs_id.

> diff --git a/drivers/ptp/ptp_kvm_x86.c b/drivers/ptp/ptp_kvm_x86.c
> index 6cea4fe39bcfe..4dd55c60d2ce8 100644
> --- a/drivers/ptp/ptp_kvm_x86.c
> +++ b/drivers/ptp/ptp_kvm_x86.c

[ ... ]

> @@ -99,6 +104,14 @@ int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
>  	unsigned int version;
>  	long ret;
>  
> +	if (*cs_id == CSID_X86_TSC) {
> +		ret = kvm_arch_ptp_get_clock(tspec);
> +		if (ret)
> +			return ret;
> +		*cycle = clock_pair->tsc;
> +		return 0;
> +	}

[Severity: Medium]
On SEV-SNP guests with Secure TSC enabled, is clock_pair->tsc in the
guest's TSC domain?

On the host, kvm_pv_clock_pairing() fills it in from KVM's software model
of the guest TSC:

arch/x86/kvm/x86.c:kvm_pv_clock_pairing() {
    ...
	clock_pairing.tsc = kvm_read_l1_tsc(vcpu, cycle);
    ...
}

With Secure TSC, sev_vcpu_create() sets vcpu->arch.guest_tsc_protected.
After that, kvm_vcpu_write_tsc_offset() and __kvm_synchronize_tsc() both
return early, so l1_tsc_offset is never set:

	if (vcpu->arch.guest_tsc_protected)
		return;

The guest's real TSC uses the scale and offset from the ASP firmware. The
guest writes them into each AP VMSA in wakeup_cpu_via_vmgexit():

	if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC)) {
		vmsa->tsc_scale = snp_tsc_scale;
		vmsa->tsc_offset = snp_tsc_offset;
	}

KVM never learns that offset. The guest code also says in
vc_handle_rdtsc() that it "cannot rely on the TSC value provided by the
hypervisor".

Secure TSC guests usually run with the tsc clocksource. Neither
kvmclock_init() nor kvm_arch_ptp_init() checks for Secure TSC, so
ptp_kvm still probes on these guests. The first PTP_SYS_OFFSET_PRECISE
request gets -ENODEV from the kvm-clock attempt. Then
ptp_kvm_getcrosststamp() retries with CSID_X86_TSC and caches it:

		if (snapshot.valid && snapshot.cs_id != cs_id) {
			cs_id = snapshot.cs_id;
			WRITE_ONCE(kvm_ptp_cs_id, cs_id);

After that, get_device_system_crosststamp() compares a counter from KVM's
model domain against tk_clock_read() in the guest's hardware domain.

Won't that counter usually fall outside [cycle_last, now]? In that case
the call returns -EINVAL because history_begin is NULL, so
PTP_SYS_OFFSET_PRECISE would still fail on these guests.

If the two domains happen to be close, could it instead return a wrong
device/system time pairing with no error?

Would it make sense to skip CSID_X86_TSC when
cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC) is true? More generally,
the check could apply whenever the hypervisor does not control the guest
TSC offset and scale. TDX looks fine here, because KVM loads
l1_tsc_offset and tsc_scaling_ratio from the TDX module.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105144821.5299-1-wwasko%40nvidia.com