Re: [RFC PATCH v3 3/4] virtio-rtc: Add alarm feature

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

On 20.01.24 11:16, Parav Pandit wrote:
> 
> 
>> From: Peter Hilber <peter.hilber@opensynergy.com>
>> Sent: Monday, December 18, 2023 12:13 PM
>>
>> Add the VIRTIO_RTC_F_ALARM feature (without normative statements).
>>
>> The intended use case is: A driver needs to react when an alarm time has
>> been reached, but the driver may be in a sleep state or powered off at alarm
>> time. The alarm feature can resume and notify the driver in this case. Alarms
>> may be retained across device resets (including reset on boot).
>>
>> Peculiarities
>> -------------
>>
>> Unlike usual alarm clocks, a virtio-rtc alarm-capable clock may step
>> autonomously at any time: An alarm may change back from "expired" to "not
>> expired" before the driver has started processing an alarm notification.
>>
>> To address the above, and the device resets, define "alarm expiration"
>> in such a way that the driver always has a chance to react to an alarm, and
>> make the device always responsible for notifying the driver about an alarm
>> expiration.
>>
>> The VIRTIO_RTC_REQ_SET_ALARM_ENABLED request is there so that the
>> Linux ioctls RTC_AIE_ON and RTC_AIE_OFF only need to emit one request.
>>
>> Open Questions
>> --------------
>>
>> - A Linux driver could use the virtio-rtc real-time alarm, through the
>>   Linux RTC device class, for both CLOCK_REALTIME_ALARM and (monotonic)
>>   CLOCK_BOOTTIME_ALARM alarms. But then, after a clock step, a
>>   CLOCK_BOOTTIME_ALARM alarm may be late (or early). Perhaps this can be
>>   handled by the driver also setting a virtio-rtc monotonic alarm, so
>>   may not need to be addressed by the spec.
>>
>> - A request returning the minimum and maximum allowed alarm time might
>>   still be added.
>>
>> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
>> ---
>>  device-types/rtc/description.tex | 256 ++++++++++++++++++++++++++++++-
>>  1 file changed, 254 insertions(+), 2 deletions(-)
>>
>> diff --git a/device-types/rtc/description.tex b/device-types/rtc/description.tex
>> index 7707472f1663..fcd2f14186e6 100644
>> --- a/device-types/rtc/description.tex
>> +++ b/device-types/rtc/description.tex
>> @@ -4,6 +4,7 @@ \section{RTC Device}\label{sec:Device Types / RTC Device}
>> time. The device can provide different clocks, e.g.\ for the UTC or TAI  time
>> standards, or for physical time elapsed since some past epoch. The  driver can
>> read the clocks with simple or more accurate methods.
>> +Optionally, the driver can set an alarm.
>>
>>  \subsection{Device ID}\label{sec:Device Types / RTC Device / Device ID}
>>
>> @@ -13,13 +14,23 @@ \subsection{Virtqueues}\label{sec:Device Types / RTC
>> Device / Virtqueues}
>>
>>  \begin{description}
>>  \item[0] requestq
>> +\item[1] alarmq
>>  \end{description}
>>
>>  The driver enqueues requests to the requestq.
>>
>> +Through the alarmq, the device notifies the driver about alarm
>> +expirations. The alarmq exists only if VIRTIO_RTC_F_ALARM was
>> +negotiated.
>> +
> I think is a good example of a need of notification queue from device to driver that has multiple use as generic object.
> On NIC side also we have been thinking to have VQ that holds only the completions as currently packed and splitvq unaligned and shorter completions are performance reducing areas.
> 
> A queue which has 1 to 8 physical addresses, and which can hold 8 to 64B of notification will be useful.
> This way per descriptor posting and DMA is not needed.
> These notifications also work very fast with least amount of descriptor caching on the device.
> 
> For alarms this may not be concern at all in current form but overall, such a generic infra has multiple uses.
> So, I suggest we bring the notification queue.

IIUC this notification queue still needs to be described.

The alarmq is a dedicated queue on purpose, to make it less intrusive to
wake the driver up while the driver may be in sleep mode (by making only
the corresponding interrupt a wake-up interrupt). Sharing the queue with
non-alarm notifications might create problems in the future.

In my reply for patch 1 I proposed to add this as an alternative interface
protected by a feature bit. This interface could cover the notification
queue as well. But I am not sure that the alarm feature will be useful for
net devices.

Thanks for the comment,

Peter