Re: [RFC PATCH v3 5/7] virtio_rtc: Add PTP clocks
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. smime.p7s Description: S/MIME cryptographic signature
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
Re: [RFC PATCH v3 5/7] virtio_rtc: Add PTP clocks
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. Shouldn't get_device_system_crosststamp() pass that to its get_time_fn as a hint? 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. smime.p7s Description: S/MIME cryptographic signature
[RFC PATCH v3 5/7] virtio_rtc: Add PTP clocks
Expose the virtio_rtc clocks as PTP clocks to userspace, similar to
ptp_kvm. virtio_rtc can expose multiple clocks, e.g. a UTC clock and a
monotonic clock. Userspace should distinguish different clocks through the
name assigned by the driver. A udev rule such as the following can be used
to get a symlink /dev/ptp_virtio to the UTC clock:
SUBSYSTEM=="ptp", ATTR{clock_name}=="Virtio PTP UTC", SYMLINK +=
"ptp_virtio"
The preferred PTP clock reading method is ioctl PTP_SYS_OFFSET_PRECISE2,
through the ptp_clock_info.getcrosststamp() op. For now,
PTP_SYS_OFFSET_PRECISE2 will return -EOPNOTSUPP through a weak function.
PTP_SYS_OFFSET_PRECISE2 requires cross-timestamping support for specific
clocksources, which will be added in the following. If the clocksource
specific code is enabled, check that the Virtio RTC device supports the
respective HW counter before obtaining an actual cross-timestamp from the
Virtio device.
The Virtio RTC device response time may be higher than the timekeeper
seqcount increment interval. Therefore, obtain the cross-timestamp before
calling get_device_system_crosststamp().
As a fallback, support the ioctl PTP_SYS_OFFSET_EXTENDED2 for all
platforms.
Assume that concurrency issues during PTP clock removal are avoided by the
posix_clock framework.
Kconfig recursive dependencies prevent virtio_rtc from implicitly enabling
PTP_1588_CLOCK, therefore just warn the user if PTP_1588_CLOCK is not
available. Since virtio_rtc should in the future also expose clocks as RTC
class devices, do not have VIRTIO_RTC depend on PTP_1588_CLOCK.
Signed-off-by: Peter Hilber
---
Notes:
v3:
- don't guard cross-timestamping with feature bit (spec v3)
- reduce clock id to 16 bits (spec v3)
v2:
- Depend on prerequisite patch series "treewide: Use clocksource id for
get_device_system_crosststamp()".
- Check clocksource id before sending crosststamp message to device.
- Do not support multiple hardware counters at runtime any more, since
distinction of Arm physical and virtual counter appears unneeded after
discussion with Marc Zyngier.
drivers/virtio/Kconfig | 23 +-
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_rtc_driver.c | 131 +-
drivers/virtio/virtio_rtc_internal.h | 46
drivers/virtio/virtio_rtc_ptp.c | 342 +++
5 files changed, 539 insertions(+), 4 deletions(-)
create mode 100644 drivers/virtio/virtio_rtc_ptp.c
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 834dd14bc070..8542b2f20201 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -179,11 +179,32 @@ config VIRTIO_RTC
depends on PTP_1588_CLOCK_OPTIONAL
help
This driver provides current time from a Virtio RTC device. The driver
-provides the time through one or more clocks.
+provides the time through one or more clocks. The Virtio RTC PTP
+clocks must be enabled to expose the clocks to userspace.
To compile this code as a module, choose M here: the module will be
called virtio_rtc.
If unsure, say M.
+if VIRTIO_RTC
+
+comment "WARNING: Consider enabling VIRTIO_RTC_PTP."
+ depends on !VIRTIO_RTC_PTP
+
+comment "Enable PTP_1588_CLOCK in order to enable VIRTIO_RTC_PTP."
+ depends on PTP_1588_CLOCK=n
+
+config VIRTIO_RTC_PTP
+ bool "Virtio RTC PTP clocks"
+ default y
+ depends on PTP_1588_CLOCK
+ help
+This exposes any Virtio RTC clocks as PTP Hardware Clocks (PHCs) to
+userspace.
+
+If unsure, say Y.
+
+endif # VIRTIO_RTC
+
endif # VIRTIO_MENU
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index f760414ed6ab..4d48cbcae6bb 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_VIRTIO_MEM) += virtio_mem.o
obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
obj-$(CONFIG_VIRTIO_RTC) += virtio_rtc.o
virtio_rtc-y := virtio_rtc_driver.o
+virtio_rtc-$(CONFIG_VIRTIO_RTC_PTP) += virtio_rtc_ptp.o
diff --git a/drivers/virtio/virtio_rtc_driver.c
b/drivers/virtio/virtio_rtc_driver.c
index ef1ea14b3bec..c331b7383285 100644
--- a/drivers/virtio/virtio_rtc_driver.c
+++ b/drivers/virtio/virtio_rtc_driver.c
@@ -35,11 +35,16 @@ struct viortc_vq {
* struct viortc_dev - virtio_rtc device data
* @vdev: virtio device
* @vqs: virtqueues
+ * @clocks_to_unregister: Clock references, which are only used during device
+ *removal.
+ * For other uses, there would be a race between device
+ * creation and setting the pointers here.
* @num_clocks: # of virtio_rtc clocks
*/
struct viortc_dev {
struct virtio_device *vdev;
struct viortc_vq vqs[VIORTC_MAX_NR_QUEUES];
+ struct viortc_ptp_clock **clocks_to_unregister;
u16 num_clocks;
};
@@ -626,6 +631,109 @@ int vior
