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

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

[Wojtek Wasko]

The KVM clock-pairing hypercall returns host wall time paired with a
guest TSC value. Cross-timestamping needs that counter expressed in the
guest's active clocksource domain.

ptp_kvm always converts the paired TSC value to kvm-clock units and
reports it as CSID_X86_KVM_CLK, so precise cross-timestamping
(PTP_SYS_OFFSET_PRECISE) fails on guests with TSC clocksource.

Return the paired TSC value directly when TSC cycles are requested and
convert it to kvm-clock units otherwise. If get_device_system_crosststamp()
rejects the counter with -ENODEV and the active clocksource differs from
the requested one, retry once using the active clocksource.

Fixes: a0e136d436de ("PTP: add kvm PTP driver")
Signed-off-by: Wojtek Wasko <wwasko@nvidia.com>
Reviewed-by: Carolina Jubran <cjubran@nvidia.com>
---
 drivers/ptp/ptp_kvm_common.c | 28 +++++++++++++++++++++++++---
 drivers/ptp/ptp_kvm_x86.c    | 13 +++++++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c
index 15ccb7dd2ed0..e11fcf961758 100644
--- a/drivers/ptp/ptp_kvm_common.c
+++ b/drivers/ptp/ptp_kvm_common.c
@@ -23,12 +23,13 @@ struct kvm_ptp_clock {
 };
 
 static DEFINE_SPINLOCK(kvm_ptp_lock);
+static enum clocksource_ids kvm_ptp_cs_id = CSID_GENERIC;
 
 static int ptp_kvm_get_time_fn(ktime_t *device_time,
 			       struct system_counterval_t *system_counter,
 			       void *ctx)
 {
-	enum clocksource_ids cs_id;
+	enum clocksource_ids cs_id = *(enum clocksource_ids *)ctx;
 	struct timespec64 tspec;
 	u64 cycle;
 	int ret;
@@ -58,8 +59,29 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time,
 static int ptp_kvm_getcrosststamp(struct ptp_clock_info *ptp,
 				  struct system_device_crosststamp *xtstamp)
 {
-	return get_device_system_crosststamp(ptp_kvm_get_time_fn, NULL,
-					     NULL, xtstamp);
+	enum clocksource_ids cs_id = READ_ONCE(kvm_ptp_cs_id);
+	struct system_time_snapshot snapshot;
+	int ret;
+
+	ret = get_device_system_crosststamp(ptp_kvm_get_time_fn, &cs_id, NULL,
+					    xtstamp);
+	/*
+	 * The paired counter value is not in the units of the current
+	 * clocksource. If the clocksource differs from the one this request
+	 * asked for, ask for the counter in the new units and retry once.
+	 */
+	if (ret == -ENODEV) {
+		ktime_get_snapshot_id(CLOCK_REALTIME, &snapshot);
+		if (snapshot.valid && snapshot.cs_id != cs_id) {
+			cs_id = snapshot.cs_id;
+			WRITE_ONCE(kvm_ptp_cs_id, cs_id);
+			ret = get_device_system_crosststamp(ptp_kvm_get_time_fn,
+							    &cs_id, NULL,
+							    xtstamp);
+		}
+	}
+
+	return ret;
 }
 
 /*
diff --git a/drivers/ptp/ptp_kvm_x86.c b/drivers/ptp/ptp_kvm_x86.c
index 6cea4fe39bcf..4dd55c60d2ce 100644
--- a/drivers/ptp/ptp_kvm_x86.c
+++ b/drivers/ptp/ptp_kvm_x86.c
@@ -92,6 +92,11 @@ int kvm_arch_ptp_get_clock(struct timespec64 *ts)
 	return 0;
 }
 
+/*
+ * *cs_id selects the clocksource @cycle is reported in: CSID_X86_TSC returns
+ * the paired TSC value as is, anything else converts it to kvm-clock units.
+ * On return, *cs_id holds the clocksource actually used.
+ */
 int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
 			      enum clocksource_ids *cs_id)
 {
@@ -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;
+	}
+
 	src = this_cpu_pvti();
 
 	do {

base-commit: 57889a8f6d49fc153aa0017d13086ab736567f8a
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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