RE: [virtio-comment] [RFC PATCH v2] virtio-rtc: Add device specification.

[Parav Pandit <parav@nvidia.com>]

> From: Peter Hilber <peter.hilber@opensynergy.com>
> Sent: Saturday, September 30, 2023 12:09 AM
> 
> On 25.09.23 11:31, Parav Pandit wrote:
> > Hi Peter,
> >
> 
> Hi Parav,
> 
> I'm sorry to be a bit late with my answer, since I only received your email via the
> mailing list, which I do not check so often.
> 
> >> From: Peter Hilber <peter.hilber@opensynergy.com>
> >> Sent: Wednesday, September 13, 2023 2:34 PM
> >
> >>> Can you please explain how 64-bit better when the number of clocks
> >>> per
> >> device is not so much?
> >>> Even with 256 or 64K clocks device wont care how the ids are issued.
> >>
> >> 64 bit width should address the potential future case where two
> >> clocks get hot- plugged and hot-unplugged at a high rate. If the
> >> device hands out a new id each time a clock is re-plugged, and if the
> >> driver suspends and resumes during this, an 8 bit id could result in the driver
> interpreting ids wrongly.
> >>
> > In the proposed spec, in a given rtc device, total number of clocks are static at
> the device discovery time.
> > Clock cannot be hotplug or unplugged dynamically.
> > Did I understand right?
> 
> Yes. The 64 bit width was just to accommodate for potential future extensions.
> This does not force the device to use effectively 64 bit wide ids. I chose 64 bit
> over 8 bit because I did not expect that this would become an issue.
> 
> >
> > If not, and if the clocks can be added/removed dynamically, if the clock id is
> not available, the device will wait to hotplug the new one, until the old one is
> removed.
> > Does one need 64K clocks under single device?
> > If so, can you please share some thoughts on example application that might
> find it useful?
> 
> The example scenario is the one which is quoted above: The device might not
> know when the driver is suspended, and continuously hand out new ids (when
> clocks are continuously re-plugged for some reason). If the driver later resumes
> at the wrong moment, by chance the same id which was in use before might
> now be assigned to another clock.
> 
If I understand correctly, inv2 we donât have a way to dynamically hotplug and unplug the clocks of _one_ virtio-rtc device.
So am I missing something?
In an arbitrary, interface, once unplug is successful, i.e. when the device exactly know that unplug is completed by the driver,
It can re-use the id.

This would work with 8-bit too.
Not that I have issue with 64-bits, but the use case for large number of clocks under single virtio-rtc device is unclear.
And if there is no use case, we should have reasonable range.

> This and similar scenarios can be prevented by using 64 bit ids.
>
Lets first establish a use case if it exists.

> 
> I understand. I am just still concerned about the dual use of the spec for the
> reasons which I gave below.
>
It is not the dual use of the spec.
It is the draft that enables common functionality for using in two use cases.

> 
> I don't know how spec sharing would be judged by the TC. At the moment, no
> other devices do this.
>
This is the first time two use cases has such a common functionality that brings.
So it is fine to introduce.
TC will evaluate and give their feedback that we can incorporate further.
Since this reduces
a. dual spec
b. dual implementation

and improves better device scaling, it is logical to cover both use cases.

 
> >> - a more complex spec impacting two devices
> >>
> > Well it is the functionality, so once you look for slightly larger use cases, overall
> it is still less complexity than writing two different specs, one for net and one for
> rtc only device.


> >
> 
> Complexity-wise it would be best to have one RTC spec, and link the RTC device
> to other devices through UUID if needed.
>
I am looking at larger use case than just rtc device.
Hence, total complexity of net device and rtc is counted in spec, and also in implementation.

So it looks better to do one time.


 
> >> - strong coupling between net and RTC driver implementation (or code
> >>   duplication)
> >>
> > Mostly no. it is the software layer. RTC layer operates over control VQ.
> > So net device will provide the needed API. Same API would be available for
> dedicated rtc device lower layer too.
> > No problem.
> >
> 
> But Virtio API will usually be time-proven, while drivers certainly have
> opportunities to get the custom API wrong.
>
I do not understand this much.
The bottom line is common functionality exists, driver maintainers can craft good APIs in their OS.
There are more than 13 drivers in one OS that I inspected, which has been peer reviewed.
So I wouldnât claim that they all got it wrong.
 
> >> - using custom interfaces (between net and RTC driver), rather than generic
> >>   interfaces
> >>
> > Multi function devices are common in nature for functionality and scale.
> > And OSes supports it already.
> >
> 
> Cf. above reply.
> 
> >> - RTC could not use device configuration
> >>
> > In your proposal all things are over cvq already.
> >
> 
> But it would prevent future use of the device configuration.
>
Not really. cvq has the commands to do the device config in your patch already.
Even for net device, implementing RW registers are hard.
The industry in various aspects are moving away from device specific configurations.
 
> >> - unrecognized issues, or future generic changes to the Virtio standard,
> >>   may inhibit future development of the RTC device or the net device
> >>
> > Unlikely as the reviews eliminate such issues.
> >
> >> - unclear responsibility and timeline for net device related spec and
> >>   proof-of-concept
> >>
> > All the responsible people of the technical committee are here.
> > In fact the requirements are more closely tracked by its stake owners for net
> compared to other devices.
> > So if you need help to find net side owner, let me know. Others and I are
> interested to have it in the net side.
> >
> 
> With "unclear responsibility" I meant to refer to the responsibility for drafting a
> spec extension for virtio-net, 

You can count me to draft the virtio-net spec for responsibility.
I am sure others would join as there is common interest across multiple device implementors to support it.
We know at least 4 stake holders have interests in it in virtio-net (Alibaba, google, marvell, nvidia).

> 
> > I would prefer to first try to upstream the RTC spec without altering
> > the net spec, while trying to avoid conflicts with the net spec. Maybe
> > most of the RTC spec could then, as a second step, also be applied to
> > the net device. The responsibility for this second step and the
> > related proof of concept work would need to be clarified. To me it
> > still seems better to not share the spec and address resource problems at the
> transport level.
> 
> >>>>>> Maybe the above use case could also be addressed by a standalone
> >>>>>> RTC device as follows:
> >>>>>>
> >>>>>> 1) The net device exports a UUID object to which the RTC device
> >>>>>> can refer to (so the drivers can link the net device to the RTC clock).
> >>>>>>
> >>>>>> 2) If needed, the RTC device aligns the lifetime of the clock to
> >>>>>> the net
> >> device.
> >>>>>> Then, clock lifetimes are no longer static, and can be aligned
> >>>>>> with the net device. Through an eventq (to be added), the RTC
> >>>>>> device announces clock creation/destruction, which can be
> >>>>>> synchronized to the net
> >>>> device state.
> >>>>>>
> >>>>> All of the infrastructure, cross device synchronization, its
> >>>>> device side
> >>>> management will come with equal complexity.
> >>>>> And we will end up with sub-optimal way to using multiple PCI devices.
> >>>>>
> >>>>
> >>>> As per my above reply, I think it would be better to tackle the
> >>>> problem at the PCI transport level...
> >>>>
> >>>>> Net already has cvq required by RTC. So cvq commands can be common.
> >>>>> Readq is additional, we need to define common between the two and
> >>>>> way to
> >>>> discover it.
> >>>>>
> >>>>
> >>>>  I would still try to avoid mixing net and RTC details. RTC should
> >>>> keep a dedicated cvq.
> >>> Can you please explain why a dedicate cvq is needed on net device?
> >>
> >> I think the concern about redundant queues could be addressed by
> >> merging the RTC readq and controlq. At the moment separating these
> >> queues would not yield performance benefits, so I think I will just
> >> merge the readq and controlq RTC queues in the next spec iteration.
> >>
> > Nice. Ok. lets have single cvq and related opcode range for RTC device and we
> will place them in the net side too.
> >
> 
> OK. In the net device, this queue should still be distinct from the net controlq,
> which has a less predictable usage pattern than RTC control requests.
>
As you describe that with cross timestamping, the predictability is not important.
Or did I misunderstand you?