On 15.06.24 10:01, David Woodhouse wrote: > On Mon, 2023-12-18 at 08:38 +0100, Peter Hilber wrote: >> >> + ret = viortc_hw_xtstamp_params(&hw_counter, &cs_id); >> + if (ret) >> + return ret; >> + >> + ktime_get_snapshot(&history_begin); >> + if (history_begin.cs_id != cs_id) >> + return -EOPNOTSUPP; > > I think you have to call ktime_get_snapshot() anyway to get a snapshot > from before your crosststamp? But I still don't much like the fact that > you need to use it to work out which cs_id is being used. The actual cs_id check is in get_device_system_crosststamp(), where it was added recently [1]. So this additional check is just verifying that the history_begin is usable. > > Shouldn't get_device_system_crosststamp() pass that to its get_time_fn > as a hint? This is unneeded in this case, since get_device_system_crosststamp() does the check already (but the driver is free to pass it through the get_time_fn parameter ctx). > > On x86, you are likely to find that history_begin.cs_id is the KVM > clock, so this will return -EOPNOTSUPP and userspace will have to fall > back to PTP_SYS_OFFSET. I note the KVM PTP clock actually *converts* a > TSC-based crosststamp to kvmclock µs for itself, so that it can report > *cs_id = CSID_X86_KVM_CLK. Not sure how I feel about that though. I'm > inclined to suggest that it shouldn't, as anyone who wants accurate > timekeeping shouldn't be using the KVM clock anyway. > > But we should at least be relatively consistent about it. ATM, the driver does indeed not have TSC support (for cross-timestamping) enabled at all, so would always use fallback. If *not* using the KVM clock, I think TSC can just be enabled by adding architecture-specific code similar to virtio_rtc_arm.c. I am not familiar with the KVM clock, but maybe it would be sufficient to allow CSID_X86_KVM_CLK as well? Thanks for the comments, Peter [1] https://git.kernel.org/torvalds/c/4b7f521229ef
