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

[David Woodhouse]

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