[virtio-dev] queue_reset register polarity to improve

[virtio-dev] queue_reset register polarity to improve

[Parav Pandit]

Hi,

A recently defined queue_reset register has a little weird definition that we should improve.
When driver initiate queue reset, it writes queue_reset = 1.
When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
Once queue reset is completed it is expected to return queue_reset = 1.
(Polarity changed twice to same value as what was driver set). See more below.

So state wise,
# q_enable, q_reset represents :
a) 0,0 -> device init time value
b) 1,0 -> vq is enabled and working
c) 1,1 -> vq is enabled, driver initiated reset
d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )

Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).

q_enable, q_reset
A) 0, 0 -> default, device init time
B) 1, 0 -> driver has enabled vq
C) 1, 1 -> driver started q reset
D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)

Parav

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: queue_reset register polarity to improve

[Michael S. Tsirkin]

On Sun, Apr 24, 2022 at 12:49:19AM +0000, Parav Pandit wrote:
> Hi,
> 
> A recently defined queue_reset register has a little weird definition that we should improve.
> When driver initiate queue reset, it writes queue_reset = 1.
> When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> Once queue reset is completed it is expected to return queue_reset = 1.
> (Polarity changed twice to same value as what was driver set). See more below.
> 
> So state wise,
> # q_enable, q_reset represents :
> a) 0,0 -> device init time value
> b) 1,0 -> vq is enabled and working
> c) 1,1 -> vq is enabled, driver initiated reset
> d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
it is not great but don't see a conflict here

> e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
this one is ugly in that state is really mostly same as (1)
but the flag values are different


> f) 1,0 -> vq is enabled and working again



It can actually be any value, the spec just says

If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
\field{QueueReset} to reset the queue, it MUST verify that the queue
has been reset by reading back \field{QueueReset} and ensuring that it
is 1.

So can be 2 or whatever, so one can distinguish between the two states.

Spec really should clarify what to do if it is not 1 (i.e. read it again
until it is 1) .


> Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> 
> q_enable, q_reset
> A) 0, 0 -> default, device init time
> B) 1, 0 -> driver has enabled vq
> C) 1, 1 -> driver started q reset
> D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> 
> Parav


Well it's been merged since November. Probably too late unless you can
convince the TC that the current feature should be abandoned
and the feature completely redesigned. Above does not look like
a deal breaker.

If we are to re-design it, I would maybe instead rework things so queue_enable can be
written to, to stop vq without a reset. Will need careful work for
transports other than PCI since those already allow writing into e.g.
QueueReady.

If possible, please open a github issue so we can track this for the
release.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: queue_reset register polarity to improve

[Jason Wang]

On Sun, Apr 24, 2022 at 2:47 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:49:19AM +0000, Parav Pandit wrote:
> > Hi,
> >
> > A recently defined queue_reset register has a little weird definition that we should improve.
> > When driver initiate queue reset, it writes queue_reset = 1.
> > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > Once queue reset is completed it is expected to return queue_reset = 1.
> > (Polarity changed twice to same value as what was driver set). See more below.
> >
> > So state wise,
> > # q_enable, q_reset represents :
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled and working
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> it is not great but don't see a conflict here
>
> > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> this one is ugly in that state is really mostly same as (1)
> but the flag values are different
>
>
> > f) 1,0 -> vq is enabled and working again
>
>
>
> It can actually be any value, the spec just says
>
> If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> \field{QueueReset} to reset the queue, it MUST verify that the queue
> has been reset by reading back \field{QueueReset} and ensuring that it
> is 1.
>
> So can be 2 or whatever, so one can distinguish between the two states.
>
> Spec really should clarify what to do if it is not 1 (i.e. read it again
> until it is 1) .
>
>
> > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> >
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> >
> > Parav
>
>
> Well it's been merged since November. Probably too late unless you can
> convince the TC that the current feature should be abandoned
> and the feature completely redesigned. Above does not look like
> a deal breaker.
>
> If we are to re-design it, I would maybe instead rework things so queue_enable can be
> written to, to stop vq without a reset.

So to make it work like MMIO's QueueReady? Btw, it's not clear what
happens if virtqueue can hold its state when write 0 to QueueReady.

Thanks

> Will need careful work for
> transports other than PCI since those already allow writing into e.g.
> QueueReady.
>
> If possible, please open a github issue so we can track this for the
> release.
>
> --
> MST
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] queue_reset register polarity to improve

[Xuan Zhuo]

On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> Hi,
>
> A recently defined queue_reset register has a little weird definition that we should improve.
> When driver initiate queue reset, it writes queue_reset = 1.
> When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> Once queue reset is completed it is expected to return queue_reset = 1.
> (Polarity changed twice to same value as what was driver set). See more below.
>
> So state wise,
> # q_enable, q_reset represents :
> a) 0,0 -> device init time value
> b) 1,0 -> vq is enabled and working
> c) 1,1 -> vq is enabled, driver initiated reset
> d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
>
> Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
>
> q_enable, q_reset
> A) 0, 0 -> default, device init time
> B) 1, 0 -> driver has enabled vq
> C) 1, 1 -> driver started q reset
> D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)

Seems to me to be two different designs, I don't see any particular value in the
conflict mentioned here, and under what circumstances would it cause any
trouble?

The design here is similar to many scenarios, such as device reset.

So if you want to change to your design, I want to know if there are other
reasons.

Thanks.

>
> Parav
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: queue_reset register polarity to improve

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, April 24, 2022 2:47 AM
> 
> On Sun, Apr 24, 2022 at 12:49:19AM +0000, Parav Pandit wrote:
> > Hi,
> >
> > A recently defined queue_reset register has a little weird definition that
> we should improve.
> > When driver initiate queue reset, it writes queue_reset = 1.
> > When device is busy resetting the queue, on this driver request, it is
> expected to return queue_reset=0.
> > Once queue reset is completed it is expected to return queue_reset = 1.
> > (Polarity changed twice to same value as what was driver set). See more
> below.
> >
> > So state wise,
> > # q_enable, q_reset represents :
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled and working
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > (conflicting definition with above #b )
> it is not great but don't see a conflict here
> 
> > e) 0,1 -> vq reset is complete in the device and VQ is now disabled
> > (again conflict with #a above )
> this one is ugly in that state is really mostly same as (1) but the flag values are
> different
Right.

> 
> 
> > f) 1,0 -> vq is enabled and working again
> 
> 
> 
> It can actually be any value, the spec just says
> 
> If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> \field{QueueReset} to reset the queue, it MUST verify that the queue has
> been reset by reading back \field{QueueReset} and ensuring that it is 1.
> 
> So can be 2 or whatever, so one can distinguish between the two states.
> 
It is sub-optimal for device to burn more than one bit to implement/return value 2, which can be communicated using single bit.
So, 0 is preferred as this the value at default reset time.

> Spec really should clarify what to do if it is not 1 (i.e. read it again until it is 1) .
> 
> 
> > Instead, I think we should have below better, consistent definition, no
> matter how queue reset occurs (init time or later).
> >
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > (communicating that its working on resetting, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches
> > the state same as device init time #A)
> >
> > Parav
> 
> 
> Well it's been merged since November. 
Merged on dec-21 to be more precise. :)

> Probably too late unless you can
> convince the TC that the current feature should be abandoned and the
> feature completely redesigned. Above does not look like a deal breaker.
> 
I don't see a need for abandoning and redesigning this feature.
Not sure if any driver+device already produced and consumed which cannot be fixed.
The spec is not released yet, we should be able to fix it.

> If we are to re-design it, I would maybe instead rework things so
> queue_enable can be written to, to stop vq without a reset. Will need careful
> work for transports other than PCI since those already allow writing into e.g.
> QueueReady.
> 
Reusing queue_enable to disable the queue require driver writing zero, device returning 1 till its enabled, and device returning 0 when done.
This can be supported using the new VIRTIO_F_RING_RESET flag.
This way we have single bit to control enablement/disablement the VQ regardless of DRIVER_OK state.

> If possible, please open a github issue so we can track this for the release.
Done at [1]

[1] https://github.com/oasis-tcs/virtio-spec/issues/139
Do we continue to discuss this over email or in github now?


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: queue_reset register polarity to improve

[Cornelia Huck]

On Sun, Apr 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Sun, Apr 24, 2022 at 12:49:19AM +0000, Parav Pandit wrote:
>> Hi,
>> 
>> A recently defined queue_reset register has a little weird definition that we should improve.
>> When driver initiate queue reset, it writes queue_reset = 1.
>> When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
>> Once queue reset is completed it is expected to return queue_reset = 1.
>> (Polarity changed twice to same value as what was driver set). See more below.
>> 
>> So state wise,
>> # q_enable, q_reset represents :
>> a) 0,0 -> device init time value
>> b) 1,0 -> vq is enabled and working
>> c) 1,1 -> vq is enabled, driver initiated reset
>> d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> it is not great but don't see a conflict here
>
>> e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> this one is ugly in that state is really mostly same as (1)
> but the flag values are different
>
>
>> f) 1,0 -> vq is enabled and working again
>
>
>
> It can actually be any value, the spec just says
>
> If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> \field{QueueReset} to reset the queue, it MUST verify that the queue
> has been reset by reading back \field{QueueReset} and ensuring that it
> is 1.
>
> So can be 2 or whatever, so one can distinguish between the two states.
>
> Spec really should clarify what to do if it is not 1 (i.e. read it again
> until it is 1) .
>
>
>> Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
>> 
>> q_enable, q_reset
>> A) 0, 0 -> default, device init time
>> B) 1, 0 -> driver has enabled vq
>> C) 1, 1 -> driver started q reset
>> D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
>> E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
>> 
>> Parav
>
>
> Well it's been merged since November. Probably too late unless you can
> convince the TC that the current feature should be abandoned
> and the feature completely redesigned. Above does not look like
> a deal breaker.

Agreed. We can add clarifications on top, though.

>
> If we are to re-design it, I would maybe instead rework things so queue_enable can be
> written to, to stop vq without a reset. Will need careful work for
> transports other than PCI since those already allow writing into e.g.
> QueueReady.

For ccw, we have not yet added queue reset; but looking at the spec, I
notice that the queue discovery section could also benefit from some
clarifications as it stands (do we need to specify special semantics for
SET_VQ with 0? do we need to forbid interactions with the queues when
SET_VQ has succeeded, but we have not yet set up indicators? etc.)


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] Re: queue_reset register polarity to improve

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, April 25, 2022 7:42 AM

> >
> > Well it's been merged since November. Probably too late unless you can
> > convince the TC that the current feature should be abandoned and the
> > feature completely redesigned. Above does not look like a deal
> > breaker.
> 
> Agreed. We can add clarifications on top, though.

I replied to Michael in other thread.
Feature was merged in Dec-21.
It is likely not too late to fix it now, instead of keeping this behavior for next 10 years.
Are there any devices, drivers that have adopted and broken due to this?
If not, lets please fix it.
Spec is not released, right?

[virtio-dev] Re: queue_reset register polarity to improve

[Michael S. Tsirkin]

On Mon, Apr 25, 2022 at 10:06:04AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Sunday, April 24, 2022 2:47 AM
> > 
> > On Sun, Apr 24, 2022 at 12:49:19AM +0000, Parav Pandit wrote:
> > > Hi,
> > >
> > > A recently defined queue_reset register has a little weird definition that
> > we should improve.
> > > When driver initiate queue reset, it writes queue_reset = 1.
> > > When device is busy resetting the queue, on this driver request, it is
> > expected to return queue_reset=0.
> > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > (Polarity changed twice to same value as what was driver set). See more
> > below.
> > >
> > > So state wise,
> > > # q_enable, q_reset represents :
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled and working
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > > (conflicting definition with above #b )
> > it is not great but don't see a conflict here
> > 
> > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled
> > > (again conflict with #a above )
> > this one is ugly in that state is really mostly same as (1) but the flag values are
> > different
> Right.
> 
> > 
> > 
> > > f) 1,0 -> vq is enabled and working again
> > 
> > 
> > 
> > It can actually be any value, the spec just says
> > 
> > If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> > \field{QueueReset} to reset the queue, it MUST verify that the queue has
> > been reset by reading back \field{QueueReset} and ensuring that it is 1.
> > 
> > So can be 2 or whatever, so one can distinguish between the two states.
> > 
> It is sub-optimal for device to burn more than one bit to implement/return value 2, which can be communicated using single bit.
> So, 0 is preferred as this the value at default reset time.
> 
> > Spec really should clarify what to do if it is not 1 (i.e. read it again until it is 1) .
> > 
> > 
> > > Instead, I think we should have below better, consistent definition, no
> > matter how queue reset occurs (init time or later).
> > >
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > > (communicating that its working on resetting, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches
> > > the state same as device init time #A)
> > >
> > > Parav
> > 
> > 
> > Well it's been merged since November. 
> Merged on dec-21 to be more precise. :)
> 
> > Probably too late unless you can
> > convince the TC that the current feature should be abandoned and the
> > feature completely redesigned. Above does not look like a deal breaker.
> > 
> I don't see a need for abandoning and redesigning this feature.
> Not sure if any driver+device already produced and consumed which cannot be fixed.
> The spec is not released yet, we should be able to fix it.
> 
> > If we are to re-design it, I would maybe instead rework things so
> > queue_enable can be written to, to stop vq without a reset. Will need careful
> > work for transports other than PCI since those already allow writing into e.g.
> > QueueReady.
> > 
> Reusing queue_enable to disable the queue require driver writing zero, device returning 1 till its enabled, and device returning 0 when done.
> This can be supported using the new VIRTIO_F_RING_RESET flag.
> This way we have single bit to control enablement/disablement the VQ regardless of DRIVER_OK state.

I'd say it would be more like VIRTIO_F_RING_DISABLE.

> > If possible, please open a github issue so we can track this for the release.
> Done at [1]
> 
> [1] https://github.com/oasis-tcs/virtio-spec/issues/139
> Do we continue to discuss this over email or in github now?

Email on as per the TC bylaws.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: queue_reset register polarity to improve

[Michael S. Tsirkin]

On Mon, Apr 25, 2022 at 12:01:21PM +0000, Parav Pandit wrote:
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Monday, April 25, 2022 7:42 AM
> 
> > >
> > > Well it's been merged since November. Probably too late unless you can
> > > convince the TC that the current feature should be abandoned and the
> > > feature completely redesigned. Above does not look like a deal
> > > breaker.
> > 
> > Agreed. We can add clarifications on top, though.
> 
> I replied to Michael in other thread.
> Feature was merged in Dec-21.

It has been under review since August of that year though and underwent
several revisions.

> It is likely not too late to fix it now, instead of keeping this behavior for next 10 years.
> Are there any devices, drivers that have adopted and broken due to this?

Hard to say. Even just speaking about open source drivers patches
have been on linux mailing list already, and I was planning on
including them in linux-next for meging a month from now.

> If not, lets please fix it.

Question is what exactly is the advantage of making changes now.  If we
are trying to save bits then maybe an alternative that uses queue enable
instead should be worked on.


> Spec is not released, right?

Kind of yes. The vote to release for a review is technically ongoing,
but practically 6 out of 8 voting members already cast a ballot and
voted yes.
The best way forward if you think the issue is important enough to delay
the release is probably working on a proposal for a change meanwhile and
submitting it as part of the 30 day public review. Since the change
seems material this would imply another 15 day public review period,
delaying the release by about a month in total.

As part of that I would suggest explaining the motivation for the
change beyond the simple "seems a bit cleaner".


-- 
MST

Re: [virtio-dev] Re: queue_reset register polarity to improve

[Michael S. Tsirkin]

On Sun, Apr 24, 2022 at 03:01:04PM +0800, Jason Wang wrote:
> On Sun, Apr 24, 2022 at 2:47 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Apr 24, 2022 at 12:49:19AM +0000, Parav Pandit wrote:
> > > Hi,
> > >
> > > A recently defined queue_reset register has a little weird definition that we should improve.
> > > When driver initiate queue reset, it writes queue_reset = 1.
> > > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > (Polarity changed twice to same value as what was driver set). See more below.
> > >
> > > So state wise,
> > > # q_enable, q_reset represents :
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled and working
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > it is not great but don't see a conflict here
> >
> > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> > this one is ugly in that state is really mostly same as (1)
> > but the flag values are different
> >
> >
> > > f) 1,0 -> vq is enabled and working again
> >
> >
> >
> > It can actually be any value, the spec just says
> >
> > If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> > \field{QueueReset} to reset the queue, it MUST verify that the queue
> > has been reset by reading back \field{QueueReset} and ensuring that it
> > is 1.
> >
> > So can be 2 or whatever, so one can distinguish between the two states.
> >
> > Spec really should clarify what to do if it is not 1 (i.e. read it again
> > until it is 1) .
> >
> >
> > > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> > >
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> > >
> > > Parav
> >
> >
> > Well it's been merged since November. Probably too late unless you can
> > convince the TC that the current feature should be abandoned
> > and the feature completely redesigned. Above does not look like
> > a deal breaker.
> >
> > If we are to re-design it, I would maybe instead rework things so queue_enable can be
> > written to, to stop vq without a reset.
> 
> So to make it work like MMIO's QueueReady?

Basically, yes.

> Btw, it's not clear what
> happens if virtqueue can hold its state when write 0 to QueueReady.
> 
> Thanks

Yes, this is an area that needs work.

> > Will need careful work for
> > transports other than PCI since those already allow writing into e.g.
> > QueueReady.
> >
> > If possible, please open a github issue so we can track this for the
> > release.
> >
> > --
> > MST
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] Re: queue_reset register polarity to improve

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, April 25, 2022 9:43 AM
> 
> Question is what exactly is the advantage of making changes now.  If we are
> trying to save bits then maybe an alternative that uses queue enable instead
> should be worked on.
> 
Bit saving by merging to queue_enable is yet another gain.
But it may be too late now as you say.
If we adopt to the alternative definition we discussed, new queue_reset bit can follow queue_enable without additional complexity.

> 
> > Spec is not released, right?
> 
> Kind of yes. The vote to release for a review is technically ongoing, but
> practically 6 out of 8 voting members already cast a ballot and voted yes.
> The best way forward if you think the issue is important enough to delay the
> release is probably working on a proposal for a change meanwhile and
> submitting it as part of the 30 day public review. Since the change seems
> material this would imply another 15 day public review period, delaying the
> release by about a month in total.
> 
> As part of that I would suggest explaining the motivation for the change beyond
> the simple "seems a bit cleaner".
> 
To me it is beyond than being cleaner.
A state machine and driver-device interface where same register values coney different things doesn't look right.

If virtio spec release process has concept of errata/ratification, I propose a minor change that doesn't affect the current release.

a. Let feature bit as is
b. On queue_reset=1, it stays 1, until queue is busy in reset
c. When queue reset completed, it goes to zero along with queue_enabled bit.

I can draft the "Fixes" text for this in existing proposed spec. This way we get right fix without much disruption.
Would that be ok?

Re: [virtio-dev] Re: queue_reset register polarity to improve

[Michael S. Tsirkin]

On Mon, Apr 25, 2022 at 02:56:39PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, April 25, 2022 9:43 AM
> > 
> > Question is what exactly is the advantage of making changes now.  If we are
> > trying to save bits then maybe an alternative that uses queue enable instead
> > should be worked on.
> > 
> Bit saving by merging to queue_enable is yet another gain.
> But it may be too late now as you say.
> If we adopt to the alternative definition we discussed, new queue_reset bit can follow queue_enable without additional complexity.
> > 
> > > Spec is not released, right?
> > 
> > Kind of yes. The vote to release for a review is technically ongoing, but
> > practically 6 out of 8 voting members already cast a ballot and voted yes.
> > The best way forward if you think the issue is important enough to delay the
> > release is probably working on a proposal for a change meanwhile and
> > submitting it as part of the 30 day public review. Since the change seems
> > material this would imply another 15 day public review period, delaying the
> > release by about a month in total.
> > 
> > As part of that I would suggest explaining the motivation for the change beyond
> > the simple "seems a bit cleaner".
> > 
> To me it is beyond than being cleaner.
> A state machine and driver-device interface where same register values coney different things doesn't look right.
> 
> If virtio spec release process has concept of errata/ratification, I propose a minor change that doesn't affect the current release.
> 
> a. Let feature bit as is
> b. On queue_reset=1, it stays 1, until queue is busy in reset
> c. When queue reset completed, it goes to zero along with queue_enabled bit.
>
> I can draft the "Fixes" text for this in existing proposed spec. This way we get right fix without much disruption.
> Would that be ok?

There's no special errata process as far as I'm aware.
Feel free to peruse
https://www.oasis-open.org/policies-guidelines/tc-process-2017-05-26/


-- 
MST

Re: [virtio-dev] queue_reset register polarity to improve

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > Hi,
> >
> > A recently defined queue_reset register has a little weird definition that we should improve.
> > When driver initiate queue reset, it writes queue_reset = 1.
> > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > Once queue reset is completed it is expected to return queue_reset = 1.
> > (Polarity changed twice to same value as what was driver set). See more below.
> >
> > So state wise,
> > # q_enable, q_reset represents :
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled and working
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> >
> > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> >
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> 
> Seems to me to be two different designs, I don't see any particular value in the
> conflict mentioned here, and under what circumstances would it cause any
> trouble?
> 
> The design here is similar to many scenarios, such as device reset.
> 
> So if you want to change to your design, I want to know if there are other
> reasons.

A benefit of Parav's suggested definition is that reading the value of
q_enable and q_reset from the device tells you the state.

The current definition relies on the driver maintaining internal state
(although this could just be assumptions in the code rather than
variables stored in memory) because it cannot determine the state by
reading q_enable and q_reset.

Introspection may not be important for regular drivers because they know
which previous operations they performed, but in some cases like driver
crash recovery or live migration introspection can be useful. That said,
they can reset the entire device to return to a known state, if
necessary.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [virtio-dev] queue_reset register polarity to improve

[Xuan Zhuo]

On Tue, 26 Apr 2022 09:48:29 +0100, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> > On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > > Hi,
> > >
> > > A recently defined queue_reset register has a little weird definition that we should improve.
> > > When driver initiate queue reset, it writes queue_reset = 1.
> > > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > (Polarity changed twice to same value as what was driver set). See more below.
> > >
> > > So state wise,
> > > # q_enable, q_reset represents :
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled and working
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> > >
> > > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> > >
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> >
> > Seems to me to be two different designs, I don't see any particular value in the
> > conflict mentioned here, and under what circumstances would it cause any
> > trouble?
> >
> > The design here is similar to many scenarios, such as device reset.
> >
> > So if you want to change to your design, I want to know if there are other
> > reasons.
>
> A benefit of Parav's suggested definition is that reading the value of
> q_enable and q_reset from the device tells you the state.
>
> The current definition relies on the driver maintaining internal state
> (although this could just be assumptions in the code rather than
> variables stored in memory) because it cannot determine the state by
> reading q_enable and q_reset.
>
> Introspection may not be important for regular drivers because they know
> which previous operations they performed, but in some cases like driver
> crash recovery or live migration introspection can be useful. That said,
> they can reset the entire device to return to a known state, if
> necessary.
>
> Stefan
>

Thanks a lot, I see the purpose of this suggestion. Thanks a lot for the
patience. This helped me.

Thanks.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] queue_reset register polarity to improve

[Michael S. Tsirkin]

On Tue, Apr 26, 2022 at 09:48:29AM +0100, Stefan Hajnoczi wrote:
> On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> > On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > > Hi,
> > >
> > > A recently defined queue_reset register has a little weird definition that we should improve.
> > > When driver initiate queue reset, it writes queue_reset = 1.
> > > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > (Polarity changed twice to same value as what was driver set). See more below.
> > >
> > > So state wise,
> > > # q_enable, q_reset represents :
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled and working
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> > >
> > > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> > >
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> > 
> > Seems to me to be two different designs, I don't see any particular value in the
> > conflict mentioned here, and under what circumstances would it cause any
> > trouble?
> > 
> > The design here is similar to many scenarios, such as device reset.
> > 
> > So if you want to change to your design, I want to know if there are other
> > reasons.
> 
> A benefit of Parav's suggested definition is that reading the value of
> q_enable and q_reset from the device tells you the state.
> 
> The current definition relies on the driver maintaining internal state
> (although this could just be assumptions in the code rather than
> variables stored in memory) because it cannot determine the state by
> reading q_enable and q_reset.
> 
> Introspection may not be important for regular drivers because they know
> which previous operations they performed, but in some cases like driver
> crash recovery or live migration introspection can be useful. That said,
> they can reset the entire device to return to a known state, if
> necessary.
> 
> Stefan

Sounds like a valid reason, probably a good idea to include in the
commit log.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] queue_reset register polarity to improve

[Xuan Zhuo]

On Tue, 26 Apr 2022 05:52:17 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Apr 26, 2022 at 09:48:29AM +0100, Stefan Hajnoczi wrote:
> > On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> > > On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > > > Hi,
> > > >
> > > > A recently defined queue_reset register has a little weird definition that we should improve.
> > > > When driver initiate queue reset, it writes queue_reset = 1.
> > > > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > > (Polarity changed twice to same value as what was driver set). See more below.
> > > >
> > > > So state wise,
> > > > # q_enable, q_reset represents :
> > > > a) 0,0 -> device init time value
> > > > b) 1,0 -> vq is enabled and working
> > > > c) 1,1 -> vq is enabled, driver initiated reset
> > > > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> > > >
> > > > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> > > >
> > > > q_enable, q_reset
> > > > A) 0, 0 -> default, device init time
> > > > B) 1, 0 -> driver has enabled vq
> > > > C) 1, 1 -> driver started q reset
> > > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> > >
> > > Seems to me to be two different designs, I don't see any particular value in the
> > > conflict mentioned here, and under what circumstances would it cause any
> > > trouble?
> > >
> > > The design here is similar to many scenarios, such as device reset.
> > >
> > > So if you want to change to your design, I want to know if there are other
> > > reasons.
> >
> > A benefit of Parav's suggested definition is that reading the value of
> > q_enable and q_reset from the device tells you the state.
> >
> > The current definition relies on the driver maintaining internal state
> > (although this could just be assumptions in the code rather than
> > variables stored in memory) because it cannot determine the state by
> > reading q_enable and q_reset.
> >
> > Introspection may not be important for regular drivers because they know
> > which previous operations they performed, but in some cases like driver
> > crash recovery or live migration introspection can be useful. That said,
> > they can reset the entire device to return to a known state, if
> > necessary.
> >
> > Stefan
>
> Sounds like a valid reason, probably a good idea to include in the
> commit log.

In this way, in addition to this reason, one is the complexity of the hardware
state. The other is register polarity, I don't understand what special effect
this problem has? It seems to be a hardware related problem.

Thanks.


>
> --
> MST
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] queue_reset register polarity to improve

[Parav Pandit]

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, April 26, 2022 5:55 AM

> > > The current definition relies on the driver maintaining internal
> > > state (although this could just be assumptions in the code rather
> > > than variables stored in memory) because it cannot determine the
> > > state by reading q_enable and q_reset.
> > >
> > > Introspection may not be important for regular drivers because they
> > > know which previous operations they performed, but in some cases
> > > like driver crash recovery or live migration introspection can be
> > > useful. That said, they can reset the entire device to return to a
> > > known state, if necessary.
> > >
> > > Stefan
> >
> > Sounds like a valid reason, probably a good idea to include in the
> > commit log.
> 
> In this way, in addition to this reason, one is the complexity of the hardware
> state. The other is register polarity, I don't understand what special effect
> this problem has? It seems to be a hardware related problem.

Not sure I follow your comment.
The proposed fix is simple, q_reset register value has to be same regardless of how the queue is reset.
Either reset VQ using q_reset register or VQ during initial reset time or device reset time.
All has to show the same value when q reset is completed regardless of hw/sw/vendor etc.

Adding to Stefan's point, current definition requires little weird state on the device side implementation too.
I showed both the example pseudo code in the commit log based on current and new polarity.
Please check that show that new polarity makes it much easier to implement.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] queue_reset register polarity to improve

[Parav Pandit]

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Sunday, April 24, 2022 3:29 AM
> >
> > A recently defined queue_reset register has a little weird definition that
> we should improve.
> > When driver initiate queue reset, it writes queue_reset = 1.
> > When device is busy resetting the queue, on this driver request, it is
> expected to return queue_reset=0.
> > Once queue reset is completed it is expected to return queue_reset = 1.
> > (Polarity changed twice to same value as what was driver set). See more
> below.
> >
> > So state wise,
> > # q_enable, q_reset represents :
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled and working
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > (conflicting definition with above #b )
> > e) 0,1 -> vq reset is complete in the device and VQ is now disabled
> > (again conflict with #a above )
> >
> > Instead, I think we should have below better, consistent definition, no
> matter how queue reset occurs (init time or later).
> >
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > (communicating that its working on resetting, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches
> > the state same as device init time #A)
> 
> Seems to me to be two different designs, I don't see any particular value in
> the conflict mentioned here, and under what circumstances would it cause
> any trouble?
> 
queue reset state doesn't go back same register value at the end of queue reset operation as what it was initial device reset operation.
And also, the special weird handling needed due to this on device side.
The rest of the commit message text deleted describes the discrepancies and also above in point #E.

> The design here is similar to many scenarios, such as device reset.
> 
Taking device reset example is not that great. As this is something exists in the code way before the spec 1.x was written.

In current form, device tells driver that queue is reset = 0) during initial time.
And when done using register method, it says queue reset is complete by saying queue_reset = 1.

This doesn't match with existing device reset semantics anyway.

No matter how to reset the device, the end result of device_status = 0, when reset is completed.

> So if you want to change to your design, I want to know if there are other
> reasons.
Those are described in the rest of the commit message and above.
Its very simple, queue reset to go back to same state as what it was during device reset time, i.e. 0.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] queue_reset register polarity to improve

[Michael S. Tsirkin]

On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > Hi,
> >
> > A recently defined queue_reset register has a little weird definition that we should improve.
> > When driver initiate queue reset, it writes queue_reset = 1.
> > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > Once queue reset is completed it is expected to return queue_reset = 1.
> > (Polarity changed twice to same value as what was driver set). See more below.
> >
> > So state wise,
> > # q_enable, q_reset represents :
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled and working
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> >
> > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> >
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> 
> Seems to me to be two different designs, I don't see any particular value in the
> conflict mentioned here, and under what circumstances would it cause any
> trouble?
> 
> The design here is similar to many scenarios, such as device reset.

Hmm. with device reset we have a reverse polarity:

	The driver SHOULD consider a driver-initiated reset complete when it
	reads \field{device status} as 0.

in what sense is the current design similar?

> So if you want to change to your design, I want to know if there are other
> reasons.
> 
> Thanks.
> 
> >
> > Parav
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: RE: [virtio-dev] queue_reset register polarity to improve

[Xuan Zhuo]

On Tue, 26 Apr 2022 12:00:24 +0000, Parav Pandit <parav@nvidia.com> wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Sunday, April 24, 2022 3:29 AM
> > >
> > > A recently defined queue_reset register has a little weird definition that
> > we should improve.
> > > When driver initiate queue reset, it writes queue_reset = 1.
> > > When device is busy resetting the queue, on this driver request, it is
> > expected to return queue_reset=0.
> > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > (Polarity changed twice to same value as what was driver set). See more
> > below.
> > >
> > > So state wise,
> > > # q_enable, q_reset represents :
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled and working
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > > (conflicting definition with above #b )
> > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled
> > > (again conflict with #a above )
> > >
> > > Instead, I think we should have below better, consistent definition, no
> > matter how queue reset occurs (init time or later).
> > >
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > > (communicating that its working on resetting, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches
> > > the state same as device init time #A)
> >
> > Seems to me to be two different designs, I don't see any particular value in
> > the conflict mentioned here, and under what circumstances would it cause
> > any trouble?
> >
> queue reset state doesn't go back same register value at the end of queue reset operation as what it was initial device reset operation.
> And also, the special weird handling needed due to this on device side.
> The rest of the commit message text deleted describes the discrepancies and also above in point #E.
>
> > The design here is similar to many scenarios, such as device reset.
> >
> Taking device reset example is not that great. As this is something exists in the code way before the spec 1.x was written.
>
> In current form, device tells driver that queue is reset = 0) during initial time.
> And when done using register method, it says queue reset is complete by saying queue_reset = 1.
>
> This doesn't match with existing device reset semantics anyway.
>
> No matter how to reset the device, the end result of device_status = 0, when reset is completed.
>
> > So if you want to change to your design, I want to know if there are other
> > reasons.
> Those are described in the rest of the commit message and above.
> Its very simple, queue reset to go back to same state as what it was during device reset time, i.e. 0.

OK, I have no problem.

Thanks for pointing this out.

Hi, Michael, what's our plan now? Will it be merged into 1.2?

Should my linux code be implemented based on this new specification?


Thanks.










---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] queue_reset register polarity to improve

[Xuan Zhuo]

On Tue, 26 Apr 2022 10:26:18 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> > On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > > Hi,
> > >
> > > A recently defined queue_reset register has a little weird definition that we should improve.
> > > When driver initiate queue reset, it writes queue_reset = 1.
> > > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > (Polarity changed twice to same value as what was driver set). See more below.
> > >
> > > So state wise,
> > > # q_enable, q_reset represents :
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled and working
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> > >
> > > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> > >
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> >
> > Seems to me to be two different designs, I don't see any particular value in the
> > conflict mentioned here, and under what circumstances would it cause any
> > trouble?
> >
> > The design here is similar to many scenarios, such as device reset.
>
> Hmm. with device reset we have a reverse polarity:
>
> 	The driver SHOULD consider a driver-initiated reset complete when it
> 	reads \field{device status} as 0.
>
> in what sense is the current design similar?

It's my understanding of register polarity that is wrong.
I just learned the process of setting device reset.

The reasons for simplifying the hardware complexity, crash recovery and live
migrationhot have convinced me.

I still don't understand "register polarity". Is it a hardware design term?
Or is it just a description of the problem, not a problem in itself.

Thanks.

>
> > So if you want to change to your design, I want to know if there are other
> > reasons.
> >
> > Thanks.
> >
> > >
> > > Parav
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: RE: [virtio-dev] queue_reset register polarity to improve

[Michael S. Tsirkin]

On Wed, Apr 27, 2022 at 04:28:43PM +0800, Xuan Zhuo wrote:
> On Tue, 26 Apr 2022 12:00:24 +0000, Parav Pandit <parav@nvidia.com> wrote:
> >
> > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Sent: Sunday, April 24, 2022 3:29 AM
> > > >
> > > > A recently defined queue_reset register has a little weird definition that
> > > we should improve.
> > > > When driver initiate queue reset, it writes queue_reset = 1.
> > > > When device is busy resetting the queue, on this driver request, it is
> > > expected to return queue_reset=0.
> > > > Once queue reset is completed it is expected to return queue_reset = 1.
> > > > (Polarity changed twice to same value as what was driver set). See more
> > > below.
> > > >
> > > > So state wise,
> > > > # q_enable, q_reset represents :
> > > > a) 0,0 -> device init time value
> > > > b) 1,0 -> vq is enabled and working
> > > > c) 1,1 -> vq is enabled, driver initiated reset
> > > > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > > > (conflicting definition with above #b )
> > > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled
> > > > (again conflict with #a above )
> > > >
> > > > Instead, I think we should have below better, consistent definition, no
> > > matter how queue reset occurs (init time or later).
> > > >
> > > > q_enable, q_reset
> > > > A) 0, 0 -> default, device init time
> > > > B) 1, 0 -> driver has enabled vq
> > > > C) 1, 1 -> driver started q reset
> > > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > > > (communicating that its working on resetting, consistent with #C)
> > > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches
> > > > the state same as device init time #A)
> > >
> > > Seems to me to be two different designs, I don't see any particular value in
> > > the conflict mentioned here, and under what circumstances would it cause
> > > any trouble?
> > >
> > queue reset state doesn't go back same register value at the end of queue reset operation as what it was initial device reset operation.
> > And also, the special weird handling needed due to this on device side.
> > The rest of the commit message text deleted describes the discrepancies and also above in point #E.
> >
> > > The design here is similar to many scenarios, such as device reset.
> > >
> > Taking device reset example is not that great. As this is something exists in the code way before the spec 1.x was written.
> >
> > In current form, device tells driver that queue is reset = 0) during initial time.
> > And when done using register method, it says queue reset is complete by saying queue_reset = 1.
> >
> > This doesn't match with existing device reset semantics anyway.
> >
> > No matter how to reset the device, the end result of device_status = 0, when reset is completed.
> >
> > > So if you want to change to your design, I want to know if there are other
> > > reasons.
> > Those are described in the rest of the commit message and above.
> > Its very simple, queue reset to go back to same state as what it was during device reset time, i.e. 0.
> 
> OK, I have no problem.
> 
> Thanks for pointing this out.
> 
> Hi, Michael, what's our plan now? Will it be merged into 1.2?
> 
> Should my linux code be implemented based on this new specification?
> 
> 
> Thanks.

So given the original commenter also feels we should make the change,
maybe the best way forward is redoing the 1.2 draft before the public
review starts. The proposal has not been finalized yet though, whether
that is practical would depend on when that is ready.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: RE: [virtio-dev] queue_reset register polarity to improve

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 27, 2022 4:30 PM
> 
> On Wed, Apr 27, 2022 at 04:28:43PM +0800, Xuan Zhuo wrote:
> > On Tue, 26 Apr 2022 12:00:24 +0000, Parav Pandit <parav@nvidia.com>
> wrote:
> > >
> > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Sent: Sunday, April 24, 2022 3:29 AM
> > > > >
> > > > > A recently defined queue_reset register has a little weird
> > > > > definition that
> > > > we should improve.
> > > > > When driver initiate queue reset, it writes queue_reset = 1.
> > > > > When device is busy resetting the queue, on this driver request,
> > > > > it is
> > > > expected to return queue_reset=0.
> > > > > Once queue reset is completed it is expected to return queue_reset
> = 1.
> > > > > (Polarity changed twice to same value as what was driver set).
> > > > > See more
> > > > below.
> > > > >
> > > > > So state wise,
> > > > > # q_enable, q_reset represents :
> > > > > a) 0,0 -> device init time value
> > > > > b) 1,0 -> vq is enabled and working
> > > > > c) 1,1 -> vq is enabled, driver initiated reset
> > > > > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > > > > (conflicting definition with above #b )
> > > > > e) 0,1 -> vq reset is complete in the device and VQ is now
> > > > > disabled (again conflict with #a above )
> > > > >
> > > > > Instead, I think we should have below better, consistent
> > > > > definition, no
> > > > matter how queue reset occurs (init time or later).
> > > > >
> > > > > q_enable, q_reset
> > > > > A) 0, 0 -> default, device init time
> > > > > B) 1, 0 -> driver has enabled vq
> > > > > C) 1, 1 -> driver started q reset
> > > > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > > > > (communicating that its working on resetting, consistent with
> > > > > #C)
> > > > > E) 0, 0 -> q_reset by device is completed, q got disabled (now
> > > > > matches the state same as device init time #A)
> > > >
> > > > Seems to me to be two different designs, I don't see any
> > > > particular value in the conflict mentioned here, and under what
> > > > circumstances would it cause any trouble?
> > > >
> > > queue reset state doesn't go back same register value at the end of
> queue reset operation as what it was initial device reset operation.
> > > And also, the special weird handling needed due to this on device side.
> > > The rest of the commit message text deleted describes the discrepancies
> and also above in point #E.
> > >
> > > > The design here is similar to many scenarios, such as device reset.
> > > >
> > > Taking device reset example is not that great. As this is something exists
> in the code way before the spec 1.x was written.
> > >
> > > In current form, device tells driver that queue is reset = 0) during initial
> time.
> > > And when done using register method, it says queue reset is complete by
> saying queue_reset = 1.
> > >
> > > This doesn't match with existing device reset semantics anyway.
> > >
> > > No matter how to reset the device, the end result of device_status = 0,
> when reset is completed.
> > >
> > > > So if you want to change to your design, I want to know if there
> > > > are other reasons.
> > > Those are described in the rest of the commit message and above.
> > > Its very simple, queue reset to go back to same state as what it was
> during device reset time, i.e. 0.
> >
> > OK, I have no problem.
> >
> > Thanks for pointing this out.
> >
> > Hi, Michael, what's our plan now? Will it be merged into 1.2?
> >
> > Should my linux code be implemented based on this new specification?
> >
> >
> > Thanks.
> 
> So given the original commenter also feels we should make the change,
> maybe the best way forward is redoing the 1.2 draft before the public review
> starts. The proposal has not been finalized yet though, whether that is
> practical would depend on when that is ready.

I am preparing v3 to address yours and Cornelia's comments.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org