Re: [RFC PATCH v3 7/7] virtio_rtc: Add RTC class driver
On 11.03.24 20:46, Alexandre Belloni wrote: > On 11/03/2024 19:28:50+0100, Peter Hilber wrote: Perhaps this might be handled by the driver also setting a virtio-rtc monotonic alarm (which uses a clock similar to CLOCK_BOOTTIME_ALARM). The virtio-rtc monotonic alarm would just be used to wake up in case it was a CLOCK_BOOTTIME_ALARM alarm. Otherwise, the behavior should not differ from other RTC class drivers. >>> >>> What I don't quite get is how this is actually related to RTCs. This >>> would be a super imprecise mechanism to get the current time and date >>> from the host to the guest which is what I think your are trying to do, >>> especially since this is not supporting UIE. >>> The host system clock may come from reading the RTC at some point in >>> time but more likely from another source so is it really the best >>> synchronization mechanism? >> >> Hello, >> >> thank you for your comments. >> >> The main motivation to have the RTC class driver is the RTC alarm >> (discussed below). >> >> As for synchronization, virtio_rtc also offers a PTP clock [1] which will >> be more precise, but which needs a user space daemon. As for RTC-based >> initial synchronization, my idea was to propose, in a second step, an >> optional op for rtc_class_ops, which would read the clock with nanosecond >> precision. This optional op could then be used in rtc_hctosys(), so there >> would be no need for UIE waiting. > > This would be a clear NAK, rtc_hctosys should use UIE to have proper > synchronisation. It currently does a very bad job reading the RTC and it > is a pity it has been mandated by systemd as useerspace is definitively > better placed to set the system time. I'm still very tempted delaying > everyone's boot by one second and make rtc_hctosys precise for all the > supported HW and not just a single driver. > OK. I plan to add a PPS feature to virtio_rtc so that it can support UIE. AFAIU this is not required for the initial driver version. Thanks for the comments, Peter [...]
Re: [RFC PATCH v3 7/7] virtio_rtc: Add RTC class driver
On 11/03/2024 19:28:50+0100, Peter Hilber wrote:
> >> Perhaps this might be handled by the driver also setting a virtio-rtc
> >> monotonic alarm (which uses a clock similar to CLOCK_BOOTTIME_ALARM).
> >> The
> >> virtio-rtc monotonic alarm would just be used to wake up in case it was
> >> a
> >> CLOCK_BOOTTIME_ALARM alarm.
> >>
> >> Otherwise, the behavior should not differ from other RTC class drivers.
> >>
> >
> > What I don't quite get is how this is actually related to RTCs. This
> > would be a super imprecise mechanism to get the current time and date
> > from the host to the guest which is what I think your are trying to do,
> > especially since this is not supporting UIE.
> > The host system clock may come from reading the RTC at some point in
> > time but more likely from another source so is it really the best
> > synchronization mechanism?
>
> Hello,
>
> thank you for your comments.
>
> The main motivation to have the RTC class driver is the RTC alarm
> (discussed below).
>
> As for synchronization, virtio_rtc also offers a PTP clock [1] which will
> be more precise, but which needs a user space daemon. As for RTC-based
> initial synchronization, my idea was to propose, in a second step, an
> optional op for rtc_class_ops, which would read the clock with nanosecond
> precision. This optional op could then be used in rtc_hctosys(), so there
> would be no need for UIE waiting.
This would be a clear NAK, rtc_hctosys should use UIE to have proper
synchronisation. It currently does a very bad job reading the RTC and it
is a pity it has been mandated by systemd as useerspace is definitively
better placed to set the system time. I'm still very tempted delaying
everyone's boot by one second and make rtc_hctosys precise for all the
supported HW and not just a single driver.
> [1]
> https://lore.kernel.org/all/20231218073849.35294-6-peter.hil...@opensynergy.com/
>
> >
> > The other thing is that I don't quite get the point of the RTC alarm
> > versus a regular timer in this context.
>
> RTC alarms allow to resume from suspend and poweroff (esp. also through
> alarmtimers), which is of interest in embedded virtualization. In my
> understanding RTC is ATM the only way to do this.
>
> (I was indeed thinking about adding an alternate alarmtimer backend for
> CLOCK_BOOTTIME_ALARM, which should deal with the CLOCK_REALTIME_ALARM vs
> CLOCK_BOOTTIME_ALARM issue which is described in the commit message.)
>
Right but this seems like a super convoluted way of getting the host to
wakeup the guest...
> >
> >
> > [...]
> >
> >> +static const struct rtc_class_ops viortc_class_with_alarm_ops = {
> >> + .read_time = viortc_class_read_time,
> >> + .read_alarm = viortc_class_read_alarm,
> >> + .set_alarm = viortc_class_set_alarm,
> >> + .alarm_irq_enable = viortc_class_alarm_irq_enable,
> >> +};
> >> +
> >> +static const struct rtc_class_ops viortc_class_no_alarm_ops = {
> >> + .read_time = viortc_class_read_time,
> >> +};
> >> +
> >
> > [...]
> >
> >> +/**
> >> +/**
> >> + * viortc_class_init() - init RTC class wrapper and device
> >> + * @viortc: device data
> >> + * @vio_clk_id: virtio_rtc clock id
> >> + * @have_alarm: expose alarm ops
> >> + * @parent_dev: virtio device
> >> + *
> >> + * Context: Process context.
> >> + * Return: RTC class wrapper on success, ERR_PTR otherwise.
> >> + */
> >> +struct viortc_class *viortc_class_init(struct viortc_dev *viortc,
> >> + u16 vio_clk_id, bool have_alarm,
> >> + struct device *parent_dev)
> >> +{
> >> + struct viortc_class *viortc_class;
> >> + struct rtc_device *rtc;
> >> +
> >> + viortc_class =
> >> + devm_kzalloc(parent_dev, sizeof(*viortc_class),
> >> GFP_KERNEL);
> >> + if (!viortc_class)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + viortc_class->viortc = viortc;
> >> +
> >> + rtc = devm_rtc_allocate_device(parent_dev);
> >> + if (IS_ERR(rtc))
> >> + return ERR_PTR(PTR_ERR(rtc));
> >> +
> >> + viortc_class->rtc = rtc;
> >> +
> >> + clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> >> +
> >> + rtc->ops = have_alarm ? _class_with_alarm_ops :
> >> + _class_no_alarm_ops;
> >
> > Don't do this, simply clear the alarm feature.
> >
>
> OK (sorry, was obviously very inelegant).
>
> Best regards,
>
> Peter
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [RFC PATCH v3 7/7] virtio_rtc: Add RTC class driver
On 08.03.24 18:03, Alexandre Belloni wrote:
> Hello,
>
> I'll start by saying that I'm sorry, I have a very very high level
> knowledge about what virtio is.
>
> On 18/12/2023 08:38:45+0100, Peter Hilber wrote:
>> Expose the virtio-rtc UTC clock as an RTC clock to userspace, if it is
>> present. Support RTC alarm if the virtio-rtc alarm feature is present.
>> The
>> virtio-rtc device signals an alarm by marking an alarmq buffer as used.
>>
>> Peculiarities
>> -
>>
>> A virtio-rtc clock is a bit special for an RTC clock in that
>>
>> - the clock may step (also backwards) autonomously at any time and
>>
>> - the device, and its notification mechanism, will be reset during boot
>> or
>> resume from sleep.
>>
>> The virtio-rtc device avoids that the driver might miss an alarm. The
>> device signals an alarm whenever the clock has reached or passed the
>> alarm
>> time, and also when the device is reset (on boot or resume from sleep),
>> if
>> the alarm time is in the past.
>>
>> Open Issue
>> --
>>
>> The CLOCK_BOOTTIME_ALARM will use the RTC clock to wake up from sleep,
>> and
>> implicitly assumes that no RTC clock steps will occur during sleep. The
>> RTC
>> class driver does not know whether the current alarm is a real-time
>> alarm
>> or a boot-time alarm.
>>
>> Perhaps this might be handled by the driver also setting a virtio-rtc
>> monotonic alarm (which uses a clock similar to CLOCK_BOOTTIME_ALARM).
>> The
>> virtio-rtc monotonic alarm would just be used to wake up in case it was
>> a
>> CLOCK_BOOTTIME_ALARM alarm.
>>
>> Otherwise, the behavior should not differ from other RTC class drivers.
>>
>
> What I don't quite get is how this is actually related to RTCs. This
> would be a super imprecise mechanism to get the current time and date
> from the host to the guest which is what I think your are trying to do,
> especially since this is not supporting UIE.
> The host system clock may come from reading the RTC at some point in
> time but more likely from another source so is it really the best
> synchronization mechanism?
Hello,
thank you for your comments.
The main motivation to have the RTC class driver is the RTC alarm
(discussed below).
As for synchronization, virtio_rtc also offers a PTP clock [1] which will
be more precise, but which needs a user space daemon. As for RTC-based
initial synchronization, my idea was to propose, in a second step, an
optional op for rtc_class_ops, which would read the clock with nanosecond
precision. This optional op could then be used in rtc_hctosys(), so there
would be no need for UIE waiting.
[1]
https://lore.kernel.org/all/20231218073849.35294-6-peter.hil...@opensynergy.com/
>
> The other thing is that I don't quite get the point of the RTC alarm
> versus a regular timer in this context.
RTC alarms allow to resume from suspend and poweroff (esp. also through
alarmtimers), which is of interest in embedded virtualization. In my
understanding RTC is ATM the only way to do this.
(I was indeed thinking about adding an alternate alarmtimer backend for
CLOCK_BOOTTIME_ALARM, which should deal with the CLOCK_REALTIME_ALARM vs
CLOCK_BOOTTIME_ALARM issue which is described in the commit message.)
>
>
> [...]
>
>> +static const struct rtc_class_ops viortc_class_with_alarm_ops = {
>> +.read_time = viortc_class_read_time,
>> +.read_alarm = viortc_class_read_alarm,
>> +.set_alarm = viortc_class_set_alarm,
>> +.alarm_irq_enable = viortc_class_alarm_irq_enable,
>> +};
>> +
>> +static const struct rtc_class_ops viortc_class_no_alarm_ops = {
>> +.read_time = viortc_class_read_time,
>> +};
>> +
>
> [...]
>
>> +/**
>> +/**
>> + * viortc_class_init() - init RTC class wrapper and device
>> + * @viortc: device data
>> + * @vio_clk_id: virtio_rtc clock id
>> + * @have_alarm: expose alarm ops
>> + * @parent_dev: virtio device
>> + *
>> + * Context: Process context.
>> + * Return: RTC class wrapper on success, ERR_PTR otherwise.
>> + */
>> +struct viortc_class *viortc_class_init(struct viortc_dev *viortc,
>> + u16 vio_clk_id, bool have_alarm,
>> + struct device *parent_dev)
>> +{
>> +struct viortc_class *viortc_class;
>> +struct rtc_device *rtc;
>> +
>> +viortc_class =
>> +devm_kzalloc(parent_dev, sizeof(*viortc_class),
>> GFP_KERNEL);
>> +if (!viortc_class)
>> +return ERR_PTR(-ENOMEM);
>> +
>> +viortc_class->viortc = viortc;
>> +
>> +rtc = devm_rtc_allocate_device(parent_dev);
>> +if (IS_ERR(rtc))
>> +return ERR_PTR(PTR_ERR(rtc));
>> +
>> +viortc_class->rtc = rtc;
>> +
>> +clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
>> +
>> +rtc->ops = have_alarm ? _class_with_alarm_ops :
>> +_class_no_alarm_ops;
>
> Don't do this, simply clear the alarm feature.
>
OK (sorry, was obviously very inelegant).
Best regards,
Peter
Re: [RFC PATCH v3 7/7] virtio_rtc: Add RTC class driver
Hello,
I'll start by saying that I'm sorry, I have a very very high level
knowledge about what virtio is.
On 18/12/2023 08:38:45+0100, Peter Hilber wrote:
> Expose the virtio-rtc UTC clock as an RTC clock to userspace, if it is
> present. Support RTC alarm if the virtio-rtc alarm feature is present. The
> virtio-rtc device signals an alarm by marking an alarmq buffer as used.
>
> Peculiarities
> -
>
> A virtio-rtc clock is a bit special for an RTC clock in that
>
> - the clock may step (also backwards) autonomously at any time and
>
> - the device, and its notification mechanism, will be reset during boot or
> resume from sleep.
>
> The virtio-rtc device avoids that the driver might miss an alarm. The
> device signals an alarm whenever the clock has reached or passed the alarm
> time, and also when the device is reset (on boot or resume from sleep), if
> the alarm time is in the past.
>
> Open Issue
> --
>
> The CLOCK_BOOTTIME_ALARM will use the RTC clock to wake up from sleep, and
> implicitly assumes that no RTC clock steps will occur during sleep. The RTC
> class driver does not know whether the current alarm is a real-time alarm
> or a boot-time alarm.
>
> Perhaps this might be handled by the driver also setting a virtio-rtc
> monotonic alarm (which uses a clock similar to CLOCK_BOOTTIME_ALARM). The
> virtio-rtc monotonic alarm would just be used to wake up in case it was a
> CLOCK_BOOTTIME_ALARM alarm.
>
> Otherwise, the behavior should not differ from other RTC class drivers.
>
What I don't quite get is how this is actually related to RTCs. This
would be a super imprecise mechanism to get the current time and date
from the host to the guest which is what I think your are trying to do,
especially since this is not supporting UIE.
The host system clock may come from reading the RTC at some point in
time but more likely from another source so is it really the best
synchronization mechanism?
The other thing is that I don't quite get the point of the RTC alarm
versus a regular timer in this context.
[...]
> +static const struct rtc_class_ops viortc_class_with_alarm_ops = {
> + .read_time = viortc_class_read_time,
> + .read_alarm = viortc_class_read_alarm,
> + .set_alarm = viortc_class_set_alarm,
> + .alarm_irq_enable = viortc_class_alarm_irq_enable,
> +};
> +
> +static const struct rtc_class_ops viortc_class_no_alarm_ops = {
> + .read_time = viortc_class_read_time,
> +};
> +
[...]
> +/**
> +/**
> + * viortc_class_init() - init RTC class wrapper and device
> + * @viortc: device data
> + * @vio_clk_id: virtio_rtc clock id
> + * @have_alarm: expose alarm ops
> + * @parent_dev: virtio device
> + *
> + * Context: Process context.
> + * Return: RTC class wrapper on success, ERR_PTR otherwise.
> + */
> +struct viortc_class *viortc_class_init(struct viortc_dev *viortc,
> +u16 vio_clk_id, bool have_alarm,
> +struct device *parent_dev)
> +{
> + struct viortc_class *viortc_class;
> + struct rtc_device *rtc;
> +
> + viortc_class =
> + devm_kzalloc(parent_dev, sizeof(*viortc_class), GFP_KERNEL);
> + if (!viortc_class)
> + return ERR_PTR(-ENOMEM);
> +
> + viortc_class->viortc = viortc;
> +
> + rtc = devm_rtc_allocate_device(parent_dev);
> + if (IS_ERR(rtc))
> + return ERR_PTR(PTR_ERR(rtc));
> +
> + viortc_class->rtc = rtc;
> +
> + clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> +
> + rtc->ops = have_alarm ? _class_with_alarm_ops :
> + _class_no_alarm_ops;
Don't do this, simply clear the alarm feature.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[RFC PATCH v3 7/7] virtio_rtc: Add RTC class driver
Expose the virtio-rtc UTC clock as an RTC clock to userspace, if it is
present. Support RTC alarm if the virtio-rtc alarm feature is present. The
virtio-rtc device signals an alarm by marking an alarmq buffer as used.
Peculiarities
-
A virtio-rtc clock is a bit special for an RTC clock in that
- the clock may step (also backwards) autonomously at any time and
- the device, and its notification mechanism, will be reset during boot or
resume from sleep.
The virtio-rtc device avoids that the driver might miss an alarm. The
device signals an alarm whenever the clock has reached or passed the alarm
time, and also when the device is reset (on boot or resume from sleep), if
the alarm time is in the past.
Open Issue
--
The CLOCK_BOOTTIME_ALARM will use the RTC clock to wake up from sleep, and
implicitly assumes that no RTC clock steps will occur during sleep. The RTC
class driver does not know whether the current alarm is a real-time alarm
or a boot-time alarm.
Perhaps this might be handled by the driver also setting a virtio-rtc
monotonic alarm (which uses a clock similar to CLOCK_BOOTTIME_ALARM). The
virtio-rtc monotonic alarm would just be used to wake up in case it was a
CLOCK_BOOTTIME_ALARM alarm.
Otherwise, the behavior should not differ from other RTC class drivers.
Signed-off-by: Peter Hilber
---
Notes:
Added in v3.
drivers/virtio/Kconfig | 21 +-
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_rtc_class.c| 269 +++
drivers/virtio/virtio_rtc_driver.c | 477 ++-
drivers/virtio/virtio_rtc_internal.h | 53 +++
include/uapi/linux/virtio_rtc.h | 88 -
6 files changed, 902 insertions(+), 7 deletions(-)
create mode 100644 drivers/virtio/virtio_rtc_class.c
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index d35c728778d2..e97bb2e9eca1 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -180,7 +180,8 @@ config VIRTIO_RTC
help
This driver provides current time from a Virtio RTC device. The driver
provides the time through one or more clocks. The Virtio RTC PTP
-clocks must be enabled to expose the clocks to userspace.
+clocks and/or the Real Time Clock driver for Virtio RTC 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.
@@ -189,8 +190,8 @@ config VIRTIO_RTC
if VIRTIO_RTC
-comment "WARNING: Consider enabling VIRTIO_RTC_PTP."
- depends on !VIRTIO_RTC_PTP
+comment "WARNING: Consider enabling VIRTIO_RTC_PTP and/or VIRTIO_RTC_CLASS."
+ depends on !VIRTIO_RTC_PTP && !VIRTIO_RTC_CLASS
comment "Enable PTP_1588_CLOCK in order to enable VIRTIO_RTC_PTP."
depends on PTP_1588_CLOCK=n
@@ -218,6 +219,20 @@ config VIRTIO_RTC_ARM
If unsure, say Y.
+comment "Enable RTC_CLASS in order to enable VIRTIO_RTC_CLASS."
+ depends on RTC_CLASS=n
+
+config VIRTIO_RTC_CLASS
+ bool "Real Time Clock driver for Virtio RTC"
+ default y
+ depends on RTC_CLASS
+ help
+This exposes the Virtio RTC UTC clock as a Linux Real Time Clock. It
+only has an effect if the Virtio RTC device has a UTC clock. The Real
+Time Clock is read-only, and may support setting an alarm.
+
+If unsure, say Y.
+
endif # VIRTIO_RTC
endif # VIRTIO_MENU
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 781dff9f8822..6c26bad777db 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_VIRTIO_RTC) += virtio_rtc.o
virtio_rtc-y := virtio_rtc_driver.o
virtio_rtc-$(CONFIG_VIRTIO_RTC_PTP) += virtio_rtc_ptp.o
virtio_rtc-$(CONFIG_VIRTIO_RTC_ARM) += virtio_rtc_arm.o
+virtio_rtc-$(CONFIG_VIRTIO_RTC_CLASS) += virtio_rtc_class.o
diff --git a/drivers/virtio/virtio_rtc_class.c
b/drivers/virtio/virtio_rtc_class.c
new file mode 100644
index ..fcb694f0f9a0
--- /dev/null
+++ b/drivers/virtio/virtio_rtc_class.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * virtio_rtc RTC class driver
+ *
+ * Copyright (C) 2023 OpenSynergy GmbH
+ */
+
+#include
+#include
+#include
+
+#include
+
+#include "virtio_rtc_internal.h"
+
+/**
+ * struct viortc_class - RTC class wrapper
+ * @viortc: virtio_rtc device data
+ * @rtc: RTC device
+ * @vio_clk_id: virtio_rtc clock id
+ * @stopped: Whether RTC ops are disallowed. Access protected by rtc_lock().
+ */
+struct viortc_class {
+ struct viortc_dev *viortc;
+ struct rtc_device *rtc;
+ u16 vio_clk_id;
+ bool stopped;
+};
+
+/**
+ * viortc_class_get_locked() - get RTC class wrapper, if ops allowed
+ * @dev: virtio device
+ *
+ * Gets the RTC class wrapper from the virtio device, if it is available and
+ * ops are allowed.
+ *
+ * Context: Caller must hold rtc_lock().
+ * Return: RTC class wrapper if available and ops
