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

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

On 28.01.24 07:15, Parav Pandit wrote:
> 
> 
>> 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

[snip]

>>>> +\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.
> 

I disagree about the complexity, and prefer to keep the RTC device without
admin virtqueues. Below I am making a proposal how RTC in the net device
could likely be used with a polished resources paradigm, without creating a
dependency for RTC standardization.

Using the resources commands as RTC commands would result in an interface
which is clearly more complex than the current interface. The resources
commands are not an adequate abstraction for all device functionality which
is not a net device data path.

The only advantage to the net device (not to the RTC device) would be that
its top level interfaces have a more unified look. My proposal 4) below
would retain this advantage. With the original resources sketch, the actual
RTC-specific interface would get more complicated for the net device as
well. 

In general, virtio devices have arbitrary command types and it is possible
to add new command types through feature bits. Why should just 4 types of
commands (CREATE, MODIFY, QUERY, DESTROY) be good enough for RTC clocks
(and also not hinder RTC development in the future)? Any existing, and any
new, commands would be merged into artificial commands (typically QUERY).
The current RTC draft has at least 4 commands on a clock resource which
would need to be mapped to a single artificial QUERY command (CLOCK_CAP,
CROSS_CAP, READ, READ_CROSS).

Combining naturally distinct commands will increase the complexity for
specifying and implementing the command. Simple devices also do not
dynamically create or remove resources, so the resource paradigm is even
less useful here.

The many issues discussed below confirm that complexity actually increases,
and indicate that the resources proposal cannot be considered complete and
mature yet.

Also, admin virtqueues have explicitly been defined and implemented as a
Virtio Over PCI Bus specific interface. Therefore, they cannot be a
mandatory dependency of a general-purpose device.

I see four different options for the net device to have RTC commands:

1) Make the RTC device a member device of the net device.

2) Put the RTC requests into the controlq, as discussed previously.

3) Introduce new group administration commands which encapsulate the RTC
   requests (similar to how it was discussed for the controlq).

4) Once the resource proposal is sufficiently complete and consistent,
   encapsulate the RTC requests in resource QUERY, resource MODIFY, or
   similar commands. This must be done in such a way as to not restrict
   future extensions to the RTC device. 

   The QUERY and MODIFY commands then need to have both a command-specific
   data and command-specific result.

   Clock ID fields in the RTC requests would then correspond to
   virtio_admin_cmd_resource_cmd_hdr.rid. The clock ID field types should
   then not be smaller than the rid field.

The RTC requirements about writing to and reading from buffers appear to be
compatible with the admin virtqueue, except for the zero-padding of short
buffers in the admin virtqueue, which could also be added to RTC.

RTC could also reuse the status definitions of the admin virtqueue.

All four above options allow to standardize RTC without depending on the
resources standardization (and PoC) progress.

The latter two options should also allow putting RTC into the admin
virtqueue for the net device. For standalone use, all four use a
device-specific queue.

>> 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.
>  

I don't understand why this would be irrelevant information when discussing
about complexity and standardization progress.

>> 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.
> 

This comment was about the delay that using resources would imply on
standardizing and implementing the RTC device.

>> 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.
> 

This is not yet described in the resource proposal.

>>   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.
>  

I agree. But my point here is that proposing new non-trivial, draft stage,
transport-specific features as a prerequisite for a long proposed feature
(RTC) can unduly delay or even kill the long proposed feature.

What if e.g. capabilities and resources were now added to RTC, but in 10
months, somebody says, drop it, I have a supposedly better idea, which
cannot be conditional on a feature bit?

>> - 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.
> 

I do not understand the previous sentence.

Admin virtqueues have explicitly been defined and implemented as a Virtio
Over PCI Bus specific interface. Therefore they cannot be a mandatory
dependency of a general-purpose, PCI-independent device.

>> - 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?

Linux kernel RTC drivers report whether a (usually physical) RTC device
alarm is currently configured or not, which the Linux kernel queries i.a.
on boot or resume from sleep.

> 
> 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.
>  

As I will argue below, there will actually be a need to introduce
RTC-specific nested headers.

>>   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.

As discussed, there is currently no use case for dynamic clock
creation/removal. But it should be possible to support this use case in the
future.

> 
> 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.

I do not understand this comment. virtio_rtc_req_cross_cap is the request
from the driver, and .hw_counter is a field in the request. flags is part
of the response.

> 
> 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?
> 

As already discussed for patch v2 [8], it could be different HW counters,
e.g. TSC and Intel ART, or TSC and HPET, for the same virtio-rtc clock.

>>   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).
> 

So if there are two clocks which support alarm, neither can be destroyed
without the other because the alarmq depends on both? As a resource
management scheme, this does not make sense to me.

Also, dynamically creating/destroying virtqueues is adding even more
complexity, and not described in the resources proposal.

>>   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.
> 

I now also saw another, somewhat contradictory admin virtqueue requirement:

   The device MUST process commands on a given admin virtqueue in
   the order in which they are queued.

This is also a limitation, since it would e.g. force the device to finish
reading out a slow clock before reading a fast clock.

>>   Maybe there are more incompatibilities here; I didn't check this
>>   thoroughly.

[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.
>  

But is it then still one QUERY command, or is it effectively multiple
commands merged into one?

>>>> +\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.
> 

Every time the driver just wants to read the clock, the device is forced to
return diverse data it has on the resource as well.

Also, "resources: Add theory of operation for device resources" now says:

    ++Managing resources using administration commands using administration virtqueues
    ++are assumed to be slower operations compared to data path operations such as
    ++handling tranmitq or receiveq.

But it must be possible to implement reading the clock as a fast command. 

>> 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.
>  

But on the performance during probe.

>> 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.

What if the device doesn't support cross-timestamping? What if the driver
is not actually interested into the cross-timestamp - does the device still
need to obtain it?

Thanks for the discussion,

Peter

[8] https://lore.kernel.org/all/b59a7dda-06fe-cff9-df61-b90aa4e50836@opensynergy.com/