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

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

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


Subject: RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state



> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 27, 2022 3:28 PM
> 
> On Wed, Apr 27, 2022 at 03:57:35PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, April 27, 2022 11:44 AM
> > >
> > > On Wed, Apr 27, 2022 at 03:39:40PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > >
> > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > >
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > >
> > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit
> > > > > > > <parav@nvidia.com>
> > > wrote:
> > > > > > > >
> > > > > > > > example flow:
> > > > > > > > a) 0,0 -> device init time value
> > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > >
> > > > > > > Did you see my reply in V1? What's the reason for using
> > > > > > > write to clear behavior that is different from the device status?
> > > > > > >
> > > > > > > We can simply make this as 1, 1 here and let the driver
> > > > > > > write to
> > > > > > > 0 to reset the virtqueue.
> > > > > > >
> > > > > > > And if we do this, the queue_enable and queue_reset are
> > > > > > > always the same, then we can simply reuse queue_enable.
> > > > > > >
> > > > > > Yes, I know we can make this work using new feature bit +
> > > > > > single
> > > > > queue_enable register.
> > > > > > I replied that in v0 to Michael.
> > > > >
> > > > > A bigger question in my eyes is that down the road we might want
> > > > > to be able to stop the ring without having it lose state.
> > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > Why queue_enable and not queue_reset?
> > >
> > > If what to disable ring without reset then writing into reset seems
> > > unintuitive.
> > >
> > True. I assume you want to start the queue again later, hence the
> stop/start.
> > Make sense.
> >
> > > > to me this interface is unlikely performant and useful for such case.
> > > > When we want to pause/stop the VQ and query the state we need
> > > performant scheme, that can even work in a batch for all the VQs.
> > > > At that point programming 64 registers to pause/stop VQ without
> > > > losing
> > > state and querying its indices etc won't be scalable with register
> interface.
> > > > I imagine a AQ (likely) or some other interface.
> > > >
> > > > >
> > > > > > I was not sure how drastic that would be at this point in the
> > > > > > spec release cycle
> > > > > that Michael highlighted.
> > > > > > Hence, I proposed a minimal change fix to queue_reset register
> > > > > > given
> > > timeline.
> > > > >
> > > > > Well if accepted this proposal is going to delay the release anyway.
> > > > > If we are doing a new feature then that can love alongside the
> > > > > one that is already in the spec.
> > > > I didn't quite understand your point.
> > >
> > > I understood your "given timeline" to mean "to avoid delays in 1.2
> release".
> > Yes.
> >
> > > My point is any material change will mean a delay at this time.
> > But this is so basic.
> > It's hard to gaze at this spec for coming years and the code to see,
> > Hey sometimes 0 means disabled, sometime 0 means still enabled,
> sometime 1 means enabled, and sometimes 1 means now disabled...
> > And maintain those weird code in device side and extra state bits burning
> some expensive chip resource.
> > Is removing from 1.2 is equal delay to get is fixed in 1.3?
> > If yes, I make humble request to fix this and have errata.
> > Some of the professional standard bodies release the spec and short after
> that errata/ratification follows the release that resolve such small issues.
> > May be time for virtio spec to take this opportunity now and be bit agile on
> it.
> > Your call. :)
> 
> I would suggest waiting for results of public review. The TC can then decide
> whether to release spec as is and then another version with bug fixes, or
> delay this one.
Ok.
I will post v3 addressing Cornelia's comments.


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