On Thu, 2024-06-20 at 14:01 +0200, Peter Hilber wrote: > 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). The *check* is a different thing. As things stand, the device has to *choose* a cs_id to use, and takes a gamble on that check in get_device_system_crosststamp() throwing the crosststamp away with -ENODEV because the device picked the wrong cs_id. That's why I'm saying it would be nicer if the core code *told* the device what cs_id to use. Rather than just throwing it away if the device guesses wrong. (Yes, it would have to be considered a hint, because it could theoretically have *changed* by the time the result is obtained, just as with your code above.) > > > > 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? Sure, that's what the ptp_kvm clock does. It actually obtains a TSC reading from the "hardware", and then manually (and unconditionally) converts that to a kvmclock value so that it can return a clock pairing based on CSID_X86_KVM_CLK. Which works until the user configures the clocksource to be the TSC instead of kvmclock, and then hits that -ENODEV check and has to do the fallback. We should just tell the device which cs_id to use.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
