[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <647cfef4-bcf1-4156-b6ae-b2e089778096@opensynergy.com>
Date: Thu, 20 Jun 2024 14:01:01 +0200
From: Peter Hilber <peter.hilber@...nsynergy.com>
To: David Woodhouse <dwmw2@...radead.org>, linux-kernel@...r.kernel.org,
virtualization@...ts.linux.dev, virtio-dev@...ts.oasis-open.org
Cc: "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
Marc Zyngier <maz@...nel.org>, Mark Rutland <mark.rutland@....com>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH v3 5/7] virtio_rtc: Add PTP clocks
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
Powered by blists - more mailing lists