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

[Cornelia Huck <cohuck@redhat.com>]

On Thu, Feb 08 2024, Peter Hilber <peter.hilber@opensynergy.com> wrote:

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

Thanks for the discussion and examples here, this helps.

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

I agree -- we have something that looks close to being ready to go, and
another set of features that, while looking interesting, are nowhere
ready to be merged. I don't think we want to have the latter hold up the
former, when the complex feature still needs resources for review and
discussion _and_ can be added on top of the original device later on.

[In case you ask: I currently don't have resources myself to deal with
the admin/resource proposal. There's a reason why it just sits on my
todo list -- I'm focusing to keep at least the project per se running
with voting etc., and that's a stretch already -- apologies for the
permanent delays in that department...]

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

We can, in theory, add support for other transports as well -- but that
needs someone to actually do it, aka resources again. I'd consider
support for this on the MMIO transport to be the bare minimum.

<snipping a bunch of interesting discussion>

I'd be in favour of addressing the smaller comments for this proposal
and then go ahead with it. We always can extend things later on.