Re: [RFC PATCH v3 5/7] virtio_rtc: Add PTP clocks

[Peter Hilber <peter.hilber@opensynergy.com>]

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