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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 27, 2022 at 04:15:05PM +0000, Parav Pandit wrote:
> 
> > From: Parav Pandit
> > Sent: Wednesday, April 27, 2022 11:58 AM
> > 
> > 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 think further, it appears a real bug that requires so special handling in the device for next years to carry.
> 
> Imagine this sequence.
> 1. A virtio device is handed over to guest VM
> 2. A virtio device started queue_reset sequence,
> So register values are:
> queue_enable = 1, q_reset=1
> 2. guest VM poled the q_reset register.
> Queue_enable = 1, q_reset = 0 (because device is doing the resetting the queue)
> 3. HV suspend the VM and queried the VQ state
> VQ state returned 
> q_enable=1, q_reset = 0.
> 
> HV doesn't know what q_reset=0 mean here, is it 0 because it was never reset?
> Or it is 0 because GVM Started the reset, but reset didn't finish?
> 
> When this virtio device is restored on the other side, HV and device doesn't know how to deal with this.
> 
> A WA that all devices will implement is, not returning 0, in step_2, but return say q_reset = 0xa to indicate that its other than 1 and other than 0.
> But hey, the destination side needs to treat this special 0xa and convert to internal q_yet_busy stage.
> 
> And this answer Jason and myself why queue_enable shouldn't be overloaded for this busy_wait register.
> Hence queue_reset register seems the right choice with the fix.
> 
> Starting to think of workaround even before the spec release is not elegant.
> Lets please fix this, bug effect is expanding beyond the primary virtio device itself.

Thanks Parav. Updating the github issue with more motivation would be a
good idea.

-- 
MST