OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: RE: [RFC PATCH v3 1/4] virtio-rtc: Add initial device specification



> From: Peter Hilber <peter.hilber@opensynergy.com>
> Sent: Wednesday, January 24, 2024 9:09 PM
> 
> On 20.01.24 11:07, Parav Pandit wrote:
> > Hi Peter,
> >
> >> From: Peter Hilber <peter.hilber@opensynergy.com>
> >> Sent: Monday, December 18, 2023 12:13 PM
> >>
> >> This is a draft for a Real-Time Clock (RTC) device. The virtio-rtc
> >> device provides information about current time through one or more
> clocks.
> >>
> >> The normative statements for this device follow in the next patch.
> >>
> >> For this device, there is an RFC Linux kernel driver which is being
> >> upstreamed, and a proprietary device implementation.
> >>
> >> Open Questions
> >> ==============
> >>
> >> Arm Generic Timer Clock Types
> >> -----------------------------
> >>
> >> This spec version still distinguishes the Arm Generic Timer virtual
> >> and physical counters:
> >>
> >> 	/* Arm Generic Timer CNTVCT_EL0, Counter-timer Virtual Count
> >> register */
> >> 	#define VIRTIO_RTC_COUNTER_ARM_VIRT 0
> >> 	/* Arm Generic Timer CNTPCT_EL0, Counter-timer Physical Count
> >> register */
> >> 	#define VIRTIO_RTC_COUNTER_ARM_PHYS 1
> >>
> >> But there might be no benefit distinguishing them, as per a
> >> discussion on the RFC Linux kernel driver [4].
> >>
> >> So unless somebody identifies some use in distinguishing Arm virtual
> >> and physical counters, this distinction will be dropped in v4.
> >>
> >> virtio-rtc Functionality for Net Device
> >> ---------------------------------------
> >>
> >> There is interest into sharing virtio-rtc functionality with the
> >> virtio-net device [6]. It is proposed to encapsulate the requests resp.
> >> responses in the pseudo-fields command-specific-data resp.
> >> command-specific-result.
> >>
> >> This handles natural alignment of the payload in a way which is
> >> compatible with
> >>
> >> - existing virtio-net controlq commands, and
> >>
> >> - with the standalone virtio-rtc messages.
> >>
> >> Additional Information
> >> ======================
> >>
> >> The spec does not specify how a driver should interpret clock readings,
> esp.
> >> also not how to perform clock synchronization.
> >>
> >> The draft uses the "Timer/Clock" device id which is already part of
> >> the specification. This device id was registered a long time ago and
> >> should be unused according to the author's information. The name
> >> "RTC" was determined to be the best for a device which focuses on
> >> current time, but is up to discussion.
> >>
> >> Any feedback is appreciated very much.
> >>
> >> Changelog
> >> =========
> >>
> >> v3:
> >>
> >> - Address comments from Parav Pandit.
> >> - Split off normative requirements into a second commit [2].
> >> - Merge readq and controlq into requestq [3].
> >> - Don't guard cross-timestamping with feature bit [3].
> >> - Pad request headers to 64 bit [2].
> >> - Rename Virtio status codes to match UNIX error names [2].
> >> - Avoid Virtio status code clashes with net controlq ack values.
> >> - Reword to refer more to "requests", rather than "messages" [2].
> >> - Rephrase some sentences [2].
> >> - Use integer data types without "__" prefixes [2].
> >> - Reduce clock id width to 16 bits [5].
> >> - Make VIRTIO_RTC_FLAG_CROSS_CAP a bit mask rather than a bit index.
> >>
> >> v2:
> >>
> >> - Address comments from Cornelia Huck.
> >> - Add VIRTIO_RTC_M_CROSS_CAP message [1].
> >> - Fix various minor issues and improve wording [1].
> >> - Add several clarifications regarding device error statuses.
> >>
> >> [1] https://lists.oasis-open.org/archives/virtio-
> >> comment/202304/msg00523.html
> >> [2] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
> >>
> b90aa4e50836@opensynergy.com/t/#mffb93800fea11d6dda9e151078abedd
> >> 6ff1c0f1e
> >> [3] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
> >>
> b90aa4e50836@opensynergy.com/t/#m94efd0aa9b9c2b96a246b79ef8bfc3bf
> >> 64ebe791
> >> [4] https://lore.kernel.org/lkml/20230630171052.985577-1-
> >>
> peter.hilber@opensynergy.com/T/#m65fa1d715933360498c4e33d7225e4220
> >> 215a9d6
> >> [5] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
> >>
> b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469
> >> 048c5d8b62
> >> [6] https://lore.kernel.org/virtio-comment/b59a7dda-06fe-cff9-df61-
> >>
> b90aa4e50836@opensynergy.com/t/#mf00ce330228c28556d735eb9597469
> >> 048c5d8b62
> >>
> >> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> >> ---
> >>  content.tex                      |   3 +-
> >>  device-types/rtc/description.tex | 328
> >> +++++++++++++++++++++++++++++++
> >>  2 files changed, 330 insertions(+), 1 deletion(-)  create mode
> >> 100644 device- types/rtc/description.tex
> >>
> >> diff --git a/content.tex b/content.tex index
> >> 98b9c319a666..f2b534ce97e6 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -683,7 +683,7 @@ \chapter{Device Types}\label{sec:Device Types}
> >> \hline
> >>  16         &   GPU device       \\
> >>  \hline
> >> -17         &   Timer/Clock device \\
> >> +17         &   RTC (Timer/Clock) device \\
> >>  \hline
> >>  18         &   Input device \\
> >>  \hline
> >> @@ -769,6 +769,7 @@ \chapter{Device Types}\label{sec:Device Types}
> >> \input{device-types/scmi/description.tex}
> >>  \input{device-types/gpio/description.tex}
> >>  \input{device-types/pmem/description.tex}
> >> +\input{device-types/rtc/description.tex}
> >>
> >>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>
> >> diff --git a/device-types/rtc/description.tex
> >> b/device-types/rtc/description.tex
> >> new file mode 100644
> >> index 000000000000..abfa2206894e
> >> --- /dev/null
> >> +++ b/device-types/rtc/description.tex
> >> @@ -0,0 +1,328 @@
> >> +\section{RTC Device}\label{sec:Device Types / RTC Device}
> >> +
> >> +The RTC (Real Time Clock) device provides information about current
> >> +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.
> >> +
> >> +\subsection{Device ID}\label{sec:Device Types / RTC Device / Device
> >> +ID}
> >> +
> >> +17
> >> +
> >> +\subsection{Virtqueues}\label{sec:Device Types / RTC Device /
> >> +Virtqueues}
> >> +
> >> +\begin{description}
> >> +\item[0] requestq
> >> +\end{description}
> >> +
> >> +The driver enqueues requests to the requestq.
> >> +
> >> +\subsection{Feature bits}\label{sec:Device Types / RTC Device /
> >> +Feature bits}
> >> +
> >> +No device-specific feature bits are defined yet.
> >> +
> >> +\subsection{Device configuration layout}\label{sec:Device Types /
> >> +RTC Device / Device configuration layout}
> >> +
> >> +There is no configuration data for the device.
> >> +
> >> +\subsection{Device Initialization}\label{sec:Device Types / RTC
> >> +Device / Device Initialization}
> >> +
> >> +The device determines the set of clocks. The device can provide zero
> >> +or more clocks.
> >> +
> > Now that we bring the generic basic facilities to define device
> > capabilities and resources in [1], Num_clocks can be structured a clock
> resources.
> >
> > Please refer to max_limits in [1] as capabilities.
> > [1]
> > https://lists.oasis-open.org/archives/virtio-comment/202401/msg00095.h
> > tml
> >
> > We should bring a rtc capabilities.
> >
> 
> Hi Parav,
> 
> thank you for the suggestions! I think that it is a good idea to standardize
> message layouts. But I also think this is late and overly complex to become
> the default RTC interface. 
It takes out the complexity or functionality that RTC tries to define and puts in generic infra.
Same was done for net device for its capabilities.

> In particular, OpenSynergy has no plans at the
> moment to support administration virtqueues (and the newly proposed
> device capabilities and device resources) in the RTC prototype
> implementation.
>
For RTC and any new facilities, I believe movement is not applicable because until now there wasn't anything.
 
> At the moment I see the following problems with using device capabilities
> and resources:
> 
> - Device capabilities and resources were initially proposed last week. It
>   remains to be seen how and when they will be accepted into the standard,
>   and when the Linux kernel driver will support them. 
The first user should be able to implement it.

> Especially the
>   dynamic resource (de-)allocation seems a tricky topic to me.
> 
Ok. Lets discuss.
Also implicit device resource is also to be considered which are kind of default resource exist on the device, which driver would not create/destroy.

>   By contrast, RTC was originally proposed 10 months ago, and has had an
>   open-source driver available since 7 months. In the past it was discussed
>   that the net device could be supported through the net controlq.
>
I assume that driver is available for reference, prototype etc, purpose and not to use it in any kind of production environment before standardizing the spec.
Otherwise there is something fundamentally broken in the virtio spec standardization in 1.x era.
 
> - Administration virtqueues are only defined for Virtio Over PCI Bus. Our
>   use cases also need Virtio Over MMIO, and maybe others in the future.
> 
AQ are like any other virtqueues. So MMIO registers to expose AQ is straightforward.
Once you have it over PCI, this can be supported.

> - The requests VIRTIO_RTC_REQ_CROSS_CAP,
> VIRTIO_RTC_REQ_READ_CROSS, and
>   likely any requests added in the future do not fit into the current
>   device resource pattern (as detailed below), so would need to get their
>   own virtqueue; or the admin virtqueue would need to allow device-specific
>   commands.
>
Let me read more below.
 
>   Also, alarms would need to be addressed, perhaps as dedicated resources.
> 
If I understand correctly, alarms are created by the driver so it can be a resource.
I think the tricky part is persistent alarms across device reset.
Does the Linux driver clock has concept of persistent alarm? Or is this for some other OS?

I believe persistent alarms also falls in the implicit resource category to me.

> - For the RTC device, using the device resources adds considerable
>   implementation overhead even for the simplest of implementations, with
> no
>   direct gain.
>
Practically it is just structured in generic format than device specific requests and response headers defined in this patch.
 
>   Both capabilities and resources can change dynamically. This would
>   necessitate a number of requirements for RTC how to deal with this. Some
>   extra generic requirements would likely also be required.
> 
Device capabilities in RTC generally should not change dynamically within the device.
For example, following structs and fields which are defined in this patch belongs to capabilities.
struct virtio_rtc_resp_cfg.num_clocks.

Device resource:
Clock -> identified with the clock_id

Resource attributes (instead of cap, as resource attribute).
struct virtio_rtc_resp_clock_cap.type
struct virtio_rtc_req_cross_cap .hw_counter and flags.

Do we have M x N matching of M clocks and N counter counters?
In my view, it was M clocks and 1 hw counter of the cpu.
Can you please clarify?

>   E.g. the device resources draft does not address how resource removal
>   would interact with resource utilization in other queues (e.g. alarmq).
> 
Good point.
We do have explicit statement added in the resources draft that, 
+The device MUST complete the command with \field{status}
+VIRTIO_ADMIN_STATUS_EBUSY if the resource
+modified or destroyd is linked to other resource and if the device cannot perform
+the requested VIRTIO_ADMIN_CMD_RESOURCE_MODIFY or VIRTIO_ADMIN_CMD_RESOURCE_DESTROY
+commands.

In your case the other resource is the VQ (alarmq).
So one needs to destroy the alarmq before destroying the RTC clock resource (if it is explicit resource).

We should extend the resources definition to indicate other objects such as VQ (and not just linked resource).

>   The only direct improvement I see is that the driver can dynamically
>   indicate which clocks it wants to use, which is not present in RTC RFC
>   spec v3. However, resource-saving features such as negotiating
>   VIRTIO_RTC_REQ_READ_CROSS support (VIRTIO_RTC_F_READ_CROSS) were
>   explicitly requested to be dropped from the spec in the past.
> 
> - The admin virtqueue spec says:
> 
>           It is the responsibility of the driver to ensure strict request
>           ordering for commands, because they will be consumed with no order
>           constraints.
> 
>   But the RTC requirements mandate certain constraints for the RTC (clock)
>   read requests processing order (so that the driver will not see backwards
>   clock jumps where there are none).
> 
Yes. above requirement to be met.
One easy option is for the driver to instruct this to the device explicitly to maintain the order.
Second option is to extend the spec for strict ordering for this new device type where driver ordering cannot be done.

>   Maybe there are more incompatibilities here; I didn't check this
>   thoroughly.
> 
> So I propose to stay with the current interface as default.
> 
> It should be possible to introduce the device capabilities and resources as an
> alternative interface, protected with a feature bit. This feature bit would then
> also omit the device-specific virtqueues. The alternative interface would need
> to address at least some of the above issues, such as how to dynamically
> create and destroy clocks. The alternative interface can perhaps also omit
> some features which are not required by its users.
> 
> I added some extra comments below, but I think the above should be
> discussed first.
> 
> Best regards,
> 
> Peter
> 
> [snip]
> >> +\item[VIRTIO_RTC_REQ_CROSS_CAP] discovers whether the device
> >> supports
> >> +cross-timestamping for a particular pair of clock and hardware counter.
> >> +
> >> +\begin{lstlisting}
> >> +#define VIRTIO_RTC_REQ_CROSS_CAP 0x1002 /* message type */
> >> +
> >> +struct virtio_rtc_req_cross_cap {
> >> +	struct virtio_rtc_req_head head;
> >> +	le16 clock_id;
> >> +	le16 hw_counter;
> >> +	u8 reserved[4];
> >> +};
> >> +
> >> +#define VIRTIO_RTC_FLAG_CROSS_CAP (1 << 0)
> >> +
> >> +struct virtio_rtc_resp_cross_cap {
> >> +	struct virtio_rtc_resp_head head;
> >> +	u8 flags;
> >> +	u8 reserved[7];
> >> +};
> >> +\end{lstlisting}
> >> +
> >> +The \field{clock_id} field identifies the clock, and the
> >> +\field{hw_counter} field identifies the hardware counter, for which
> >> +cross-timestamp support is probed. The device sets flag
> >> +\field{VIRTIO_RTC_FLAG_CROSS_CAP} in the \field{flags} field if the
> >> +clock supports cross-timestamping for the particular clock and
> >> +hardware counter, and clears the flag otherwise.
> >> +
> > Hw_counter is also the resource attribute.
> >
> 
> But a clock may support multiple hw_counters; therefore the driver provides
> the hw_counter in the request. So it would not fit into the
> VIRTIO_ADMIN_CMD_RESOURCE_QUERY pattern out-of-the-box.
>
Right. We should possibly be able to extend the QUERY command to accept device type specific input if the hw_counters are in huge quantities.
 
> >> +\end{description}
> >> +
> >> +\subsubsection{Read Requests}\label{sec:Device Types / RTC Device /
> >> +Device Operation / Read Requests}
> >> +
> >> +Through \emph{read requests}, the driver requests clock readings
> >> +from the device. The driver enqueues read requests in the requestq.
> >> +The device obtains device-side clock readings and forwards these
> >> +clock readings to the driver. The driver may enhance and interpret
> >> +the clock readings through methods which are beyond the scope of
> >> +this specification.
> >> +
> > Reading the resource using generic query resource command of [2] will
> return the time value.
> > So only time definition is needed in the resource.
> > [2]
> > https://lists.oasis-open.org/archives/virtio-comment/202401/msg00096.h
> > tml
> >
> 
> Treating current time as a resource attribute appears questionable. First, it
> continuously changes, while other attributes don't. 
It is a resource_specific_data. So it is free to continuously change.
A packet counter, flow counter etc similar examples, which will also have constantly changing value.

> Second, it could also be a
> slow operation when reading e.g. from a GNSS clock attached over a slow
> interface.
>
If I understand correctly, this has no effect on structuring it as a resource of some cvq command.
 
> Also, the query resource command does not fit read request
> VIRTIO_RTC_REQ_READ_CROSS, which requires the hw_counter parameter,
> and returns extra results.
Query resource command extension should be extended to accept this resource specific data apart from rid.


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]